Refactor module_function to reduce rbi need #15249
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dduugg
force-pushed
the
module-function-cleanup
branch
from
52f7367
to
cd12c5f
Compare
dduugg
force-pushed
the
module-function-cleanup
branch
from
cd12c5f
to
f8b27e6
Compare
dduugg
commented
Apr 17, 2023
| # TODO: bump version when new Xcode macOS SDK is released | ||
| # NOTE: We only track the major version of the SDK. | ||
| ::Version.new("13") | ||
| end | ||
| private :latest_sdk_version |
There was a problem hiding this comment.
This was useless, since it only affected the instance method, which was already private. For example: https://github.com/Homebrew/brew/blob/e98bfdd/Library/Homebrew/os/mac/xcode.rb#L69
dduugg
force-pushed
the
module-function-cleanup
branch
from
f8b27e6
to
af8a422
Compare
dduugg
force-pushed
the
module-function-cleanup
branch
from
af8a422
to
df5c584
Compare
MikeMcQuaid
approved these changes
Apr 17, 2023
MikeMcQuaid
approved these changes
Apr 17, 2023
|
Thanks again @dduugg! |
dduugg
commented
Apr 17, 2023
| @@ -32,7 +32,7 @@ def initialize(regex, max_length, replacement) | |||
| # rewrite_shebang detected_python_shebang, bin/"script.py" | |||
| # | |||
| # @api public | |||
| sig { params(rewrite_info: RewriteInfo, paths: T::Array[T.any(String, Pathname)]).void } | |||
There was a problem hiding this comment.
It looks like sig validation is another victim of module_function (I had to revert this due to the instance use in Formula, but kept the type signature fix here).
Bo98
reviewed
Apr 18, 2023
| Utils::Shebang::RewriteInfo.new( | ||
| %r{^#! ?/usr/bin/(?:env )?python(?:[23](?:\.\d{1,2})?)?( |$)}, | ||
| 28, # the length of "#! /usr/bin/env pythonx.yyy " | ||
| "#{python_path}\\1", | ||
| ) | ||
| end | ||
|
|
||
| def detected_python_shebang(formula = self, use_python_from_path: false) | ||
| def self.detected_python_shebang(formula = self, use_python_from_path: false) |
There was a problem hiding this comment.
This is a mixin module as documented above and this breaks it.
See formulae which invoke include Language::Python::Shebang and call detected_python_shebang.
There was a problem hiding this comment.
Reverted in #15259, PTAL
| module_function | ||
|
|
||
| def detected_perl_shebang(formula = self) | ||
| def self.detected_perl_shebang(formula = self) |
7 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?module_functionis hard to get right, as it creates methods in multiple namespaces (a private instance and a public class method), and the resulting code needs to make sure that references resolve in both places. Aliases and visibility modifiers are also hard to get right. (It's also confusing for end users, as it creates uncertainty around which is the right way to make use of the module.)A lot of this was cleaned up when enabling typechecking, but i've ported a few more in this PR. This was primarily motivated by the desire to reduce the need for custom
.rbifiles, which have been cut by 30% (70 → 49).