-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Inline type annotations. #8896
Inline type annotations. #8896
Conversation
237add9
to
d491004
Compare
I spent more time than I'd like to admit getting the |
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.
The count of files changed and the diff stats make this PR look more intimidating than it actually is - good job on all of this, Markus! 😍 And now it'll be easier for new files to have # typed:
sigils added.
ba54f1a
to
644b6a3
Compare
644b6a3
to
73dc98d
Compare
@@ -1,3 +1,4 @@ | |||
# typed: false |
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.
These seem like a reasonable middle-ground 👍🏻
Nice work @reitermarkus! |
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.
Just curious, why don't we have all RBI files in the sorbet
directory?
I think it's easier to maintain when the Especially for the ones in |
brew style
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?Some
.rbi
are still needed for theinclude Kernel
case, since includingKernel
in actual code causes it to override default methods, i.e.to_s
with the default implementation.Removed
files.yaml
, since all files now have a# typed:
comment.Refactored some
module_function
uses toclass << self
so thatinclude Kernel
is not needed.Added a
SystemCommand::Mixin
module to reduce the need for includingKernel
in some cases where the only needed method issystem_command
.