Skip to content
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

Merged
merged 15 commits into from
Oct 13, 2020
Merged

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Oct 10, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

Some .rbi are still needed for the include Kernel case, since including Kernel 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 to class << self so that include Kernel is not needed.

Added a SystemCommand::Mixin module to reduce the need for including Kernel in some cases where the only needed method is system_command.

Sorry, something went wrong.

@issyl0
Copy link
Member

issyl0 commented Oct 10, 2020

I spent more time than I'd like to admit getting the files.yaml pruning script working. Good riddance! 😂

Copy link
Member

@issyl0 issyl0 left a 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.

@@ -1,3 +1,4 @@
# typed: false
Copy link
Member

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 👍🏻

@MikeMcQuaid
Copy link
Member

Nice work @reitermarkus!

Copy link
Contributor

@vidusheeamoli vidusheeamoli left a 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?

@reitermarkus
Copy link
Member Author

reitermarkus commented Oct 12, 2020

why don't we have all RBI files in the sorbet directory?

I think it's easier to maintain when the .rbi files are right next to the corresponding .rb files.

Especially for the ones in compat, there were already some leftover things in sorbet/ which don't exist anymore.

@reitermarkus reitermarkus merged commit bf7fe45 into Homebrew:master Oct 13, 2020
@reitermarkus reitermarkus deleted the sorbet-inline branch October 13, 2020 08:40
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 1, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants