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.

Sign up for GitHub

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

Jump to bottom

Refactor `CaskLoader::for`. #16609

Merged

reitermarkus merged 6 commits into Homebrew:master from reitermarkus:cask-loader-for

Feb 8, 2024

Member

reitermarkus commented Feb 7, 2024 •

edited

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 typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Revert #16608 and refactor CaskLoader::for to use the same logic for full and short names, which hopefully also fixes #16607 properly.

reitermarkus added 3 commits

February 7, 2024 02:02


          Revert "Merge pull request Homebrew#16608 from bevanjkay/revert-16596…

e5a6f76

…-cask-rename-migrations"

This reverts commit 5799e85, reversing
changes made to 6775171.


          Refactor CaskLoader::for.

889c3c3


          Reuse rename logic for default cask tap.

c559e67

reitermarkus requested review from Bo98, bevanjkay and apainintheneck

February 7, 2024 01:07

apainintheneck reviewed

View reviewed changes

Contributor

apainintheneck left a comment

I left a bunch of nits but of course you can ignore those. Other than the use of Tap#ensure_installed! that I'm a bit worried about this looks very good and helps clean up a lot of the code.

Library/Homebrew/cask/cask_loader.rb Show resolved Hide resolved

Library/Homebrew/cask/cask_loader.rb

Comment on lines 52 to +53

    
                  class FromContentLoader < AbstractContentLoader

                    def self.can_load?(ref)

                    def self.try_new(ref, warn: false)

Contributor

apainintheneck Feb 7, 2024

I like this approach but I am 50/50 on the name. I don't have any better ideas though.

Would this approach make it easier to streamline formula loading as well? The current #can_load? style logic doesn't really work well for formulae since we need a bunch of workarounds and extra checks when deciding what to load.

Member Author

reitermarkus Feb 7, 2024

Yes, I hope we can use the same pattern for formulae as well.

Library/Homebrew/cask/cask_loader.rb

Comment on lines -72 to +75

		@content = content.force_encoding("UTF-8")
		@content = content.dup.force_encoding("UTF-8")

Contributor

apainintheneck Feb 7, 2024

I doubt the String#dup is really necessary here. Are we relying on the original string encoding anywhere else?

Member Author

reitermarkus Feb 7, 2024

Some tests were failing here because they pass in string literals. It is surprising that this is necessary only now though.

Member Author

reitermarkus Feb 7, 2024

Oh right, we were only testing can_load? before, not the constructor.

Library/Homebrew/cask/cask_loader.rb

Comment on lines 54 to +56

                       return false unless ref.respond_to?(:to_str)
-                      content = ref.to_str
+                      content = T.unsafe(ref).to_str

Contributor

apainintheneck Feb 7, 2024

I'm a bit surprised that Sorbet can't figure this one out. sigh

Member Author

reitermarkus Feb 7, 2024

Yes, unfortunate, but it does make sense: https://sorbet.org/docs/flow-sensitive#what-about-respond_to

You cannot deduce a type by its methods alone. We could just use is_a?(String) here though, because anything else doesn't make sense anyways.

Contributor

apainintheneck Feb 8, 2024

Either way is fine for me. Thanks for explaining it to me.

Library/Homebrew/cask/cask_loader.rb Show resolved Hide resolved

Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved

Library/Homebrew/cask/cask_loader.rb Show resolved Hide resolved

Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved

apainintheneck reviewed

View reviewed changes

Library/Homebrew/tap_constants.rb Outdated Show resolved Hide resolved


          Remove unused capture group name.

bc60607

Co-authored-by: Kevin <apainintheneck@gmail.com>

reitermarkus commented

View reviewed changes

Library/Homebrew/cask/cask_loader.rb Show resolved Hide resolved

reitermarkus and others added 2 commits

February 7, 2024 14:02


          Simplify code style.

825222c

Co-authored-by: Kevin <apainintheneck@gmail.com>


          Readd debug logging.

c301b9c

reitermarkus force-pushed the cask-loader-for branch 2 times, most recently from 659932e to c301b9c Compare

February 7, 2024 14:34

reitermarkus requested a review from apainintheneck

February 7, 2024 14:36

apainintheneck approved these changes

View reviewed changes

Contributor

apainintheneck left a comment

Looks good to me now, thanks!

Library/Homebrew/cask/cask_loader.rb

Comment on lines 54 to +56

                       return false unless ref.respond_to?(:to_str)
-                      content = ref.to_str
+                      content = T.unsafe(ref).to_str

Contributor

apainintheneck Feb 8, 2024

Either way is fine for me. Thanks for explaining it to me.

MikeMcQuaid approved these changes

View reviewed changes

reitermarkus merged commit c9934ef into Homebrew:master

24 checks passed

reitermarkus deleted the cask-loader-for branch

February 8, 2024 10:43

github-actions bot added the outdated label

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.