-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use prepended modules instead of undef
for OS-specific code
#18305
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks good and makes sense to me, nice work!
I wonder if there's any way to avoid the need for the require
at the bottom of the existing class? That'd be nice if possible.
@@ -2,8 +2,10 @@ | |||
# frozen_string_literal: true | |||
|
|||
module Homebrew | |||
class Cleanup | |||
undef use_system_ruby? | |||
module CleanupLinux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module CleanupLinux | |
module Linux::Cleanup |
or
module CleanupLinux | |
module Cleanup::Linux |
or
module CleanupLinux | |
module Cleanup::LinuxExtensions |
or maybe even
module CleanupLinux | |
module OS::Linux::Cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS::Linux::Cleanup
most closely matches the path, let's go with that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, one issue with introducing OS
into the name space is that breaks resolution of e.g. OS.linux?
under Homebrew
, including in outside repos, e.g. https://github.com/Homebrew/homebrew-test-bot/blob/cb38bc3/lib/test_bot.rb#L63
We could alias ::OS
to point to Homebrew::OS
, but wanted to do a sanity check before pursuing this route any further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get a simple alias to work, but this does:
module Homebrew
module OS
class << self
extend Forwardable
def_delegators '::OS', :mac?, :linux?, :kernel_version, :kerneal_name, :unsupported_configuration?
end
end
end
We'd presumably then deprecate top-level ::OS
, given the desire to move everything under the Homebrew
namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd presumably then deprecate top-level
::OS
, given the desire to move everything under theHomebrew
namespace.
Would that deprecate OS.linux?
and OS.mac?
? If so, then that would break a lot of code, and not just in Homebrew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OS::Linux::Cleanup
most closely matches the path, let's go with that one
@dduugg sounds good, thanks!
I think having a top-level OS
even just as an alias to Homebrew::OS
would be good. As @carlocab mentioned, it's been an incredibly heavily used public API for a long time to the point that I'd probably rather never deprecate/disable/remove it but keep around the alias indefinitely (like we do with MacOS
to OS::Mac
).
If the alias isn't possible: feel free to leave this as-is and avoid the Homebrew namespace (or do so just for the extend
modules)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved all the OS extensions under the top-level OS
in the latest diff, which avoids having to do full-path resolution all over the place. I was ~forced to change OS::MacOS
to OS::Mac
, because ::MacOS
is already defined as a constant under OS
, which would create more namespace resolution errors.
Yes, that would be very nice, but right now sorbet/sorbet#5025 requires the |
2ef6464
to
197447c
Compare
4fe011a
to
466fdf6
Compare
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Relates to #17998
Switches from using
undef
, which results in (suppressed) sorbet errors, with prepended modules.The latest proposal on the thread suggested creating an interface with three implementations, rather than using the generic-OS version in the default implementation, with methods undefined in OS-specific code. In building that out, I noticed that there wasn't a consistent, common interface among the OS variations, specifically in the case of
Formula
.Thus, I've landed on OS extensions written as modules, which are then prepended to be used ahead of the generic implementations. (Note that in some cases, the modules are prepended to the singleton class, which admittedly gets pretty esoteric.)