dev-cmd/edit: Move path specific functions to Pathname #16029

abitrolly · 2023-09-24T14:01:16Z

This PR reduces edit.rb complexity to solve #16015.

Moves to Pathname the logic that checks if the path is core tap, cask or formula.
Moves failure handler when path does not exist into separate method.

This makes the code easier to read to fix the name conflict between formula and existing directory.

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?

abitrolly · 2023-09-24T19:30:04Z

CI fails with Style/Next: Use next to skip iteration. (https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/Next), but if I autofix it, it becomes Style/UnlessLogicalOperators, which is unfixable. What to do?

Library/Homebrew/dev-cmd/edit.rb

Library/Homebrew/extend/pathname.rb

apainintheneck · 2023-09-24T20:39:19Z

Thinking about this a little more I think we should check that the paths are valid before checking to see if we want to print the install warning.

abitrolly · 2023-09-25T10:32:23Z

Thinking about this a little more I think we should check that the paths are valid before checking to see if we want to print the install warning.

@apainintheneck moved path validation before checking for warning message. The code for error message is also in a new method, so the logic should be even more readable now.

The last two offences are not easy to fix, though.

 /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/dev-cmd/edit.rb:92:7: C: Style/CombinableLoops: Combine this loop with the previous loop.
      expanded_paths.each do |path| ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/dev-cmd/edit.rb:93:9: C: [Correctable] Style/Next: Use next to skip iteration.
        if (path.core_formula_path? || path.core_cask_path? || path.core_formula_tap? || path.core_cask_tap?) && ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

MikeMcQuaid

Thanks for the PR and work so far!

Library/Homebrew/extend/pathname.rb

Library/Homebrew/dev-cmd/edit.rb

https://docs.ruby-lang.org/en/2.4.0/syntax/refinements_rdoc.html

abitrolly · 2023-09-25T14:27:15Z

I guess I addressed all review comments.

MikeMcQuaid

Thanks again! A few more thoughts.

Library/Homebrew/dev-cmd/edit.rb

MikeMcQuaid · 2023-09-25T14:38:20Z

Library/Homebrew/dev-cmd/edit.rb

@@ -28,6 +54,40 @@ def edit_args
    end
  end

+  sig { params(path: Pathname, cask: T::Boolean).void }
+  def fail_with_message(path, cask)


Suggested change

def fail_with_message(path, cask)

def raise_unless_existing!(path, cask)

@abitrolly Still prefer this naming or at least something with raise and the ! to be more in keeping with typical Ruby behaviour.

Renamed to raise_with_message!

Library/Homebrew/dev-cmd/edit.rb

MikeMcQuaid · 2023-09-25T14:39:59Z

Library/Homebrew/dev-cmd/edit.rb

-        end
+      expanded_paths = args.named.to_paths
+      expanded_paths.each do |path|
+        fail_with_message(path, args.cask?) unless path.exist?


How about moving the unless path.exist? into raise_unless_existing ?

It can be done, but I would rather declare fail_with_message as T.noreturn to keep the branch flow in edit. So what is your decision?

You can still have it as void as long as the return value is not used by callers.

Library/Homebrew/dev-cmd/edit.rb

Co-authored-by: Mike McQuaid <[email protected]>

apainintheneck · 2023-09-26T05:42:39Z

I'm fine with this once the linter is happy. If it's too difficult to make the linter happy, you could just keep the original loop logic minus the select statement and add the Pathname refinements which still makes this clearer IMO.

abitrolly · 2023-09-26T07:14:43Z

@apainintheneck I reworked the warning message test to use any?, so linter is now happy. Now only typecheck is not satisfied.

abitrolly · 2023-09-26T10:49:51Z

I don't think I am able to fix the typecheck error for this code. I opened sorbet/sorbet#7350 to get better docs about what to do with refinements, but for now it is beyond my abilities.

MikeMcQuaid · 2023-09-26T12:58:53Z

I don't think I am able to fix the typecheck error for this code. I opened sorbet/sorbet#7350 to get better docs about what to do with refinements, but for now it is beyond my abilities.

CC @dduugg @reitermarkus for ideas and thoughts here on how best to handle things.

Would prefer to use Refinements if possible but if we can't with Sorbet: so be it (and then I think the old approach is better than moving to Pathname).

dduugg · 2023-09-26T17:19:32Z

I don't think I am able to fix the typecheck error for this code. I opened sorbet/sorbet#7350 to get better docs about what to do with refinements, but for now it is beyond my abilities.

CC @dduugg @reitermarkus for ideas and thoughts here on how best to handle things.

Would prefer to use Refinements if possible but if we can't with Sorbet: so be it (and then I think the old approach is better than moving to Pathname).

Genuinely curious, why do you prefer refinements @MikeMcQuaid ? IIUC they're a fix intended to scope legacy monkey-patches to where they are used, not a good idea for new code. They're banned at Shopify and Stripe.

Sorbet doesn't support refinements. The workaround I used in brew code for existing refinements was to T.bind the defs to the class being refined (example). (You may also need to add the defs to an rbi file, but it looks like the linked example didn't need to.)

abitrolly · 2023-09-27T05:32:04Z

Maybe https://ruby-doc.org/stdlib-2.2.1/libdoc/delegate/rdoc/SimpleDelegator.html can be used to wrap Pathname instead of the Refinement?

EDIT: sorbet contributors confirmed it probably won't support refinements sorbet/sorbet#7350 (comment)

MikeMcQuaid · 2023-09-27T08:00:44Z

Genuinely curious, why do you prefer refinements @MikeMcQuaid ? IIUC they're a fix intended to scope legacy monkey-patches to where they are used, not a good idea for new code. They're banned at Shopify and Stripe.

I prefer them to adding mode global monkeypatches to Pathname. I don't prefer them to e.g. the existing approach.

Library/Homebrew/dev-cmd/edit.rb

abitrolly · 2023-09-27T10:08:05Z

All green. Let me know if there is anything else left.

MikeMcQuaid

Looks good! Thanks again @abitrolly and sorry for all the back and forth.

abitrolly · 2023-09-27T12:53:45Z

No problem. I am glad it helped. )

abitrolly force-pushed the edit-simplify branch from 617272d to e31d6a2 Compare September 24, 2023 15:12

abitrolly marked this pull request as ready for review September 24, 2023 19:31

apainintheneck reviewed Sep 24, 2023

View reviewed changes

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

Library/Homebrew/extend/pathname.rb Outdated Show resolved Hide resolved

abitrolly mentioned this pull request Sep 24, 2023

brew edit --print-path --formula src prints src not the full path #16015

Closed

3 tasks

MikeMcQuaid reviewed Sep 25, 2023

View reviewed changes

Library/Homebrew/extend/pathname.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

abitrolly added 5 commits September 25, 2023 15:56

dev-cmd/edit: Move path specific functions to Pathname

1cbf5ce

Separate logic that shows warning from path detection

eae9567

Move error reporting logic for absent path to separate method

8f072dc

Check paths exist before brew install warning

a78da3e

Rename fail to more descriptive fail_with_message

63c60d2

abitrolly force-pushed the edit-simplify branch from d4a5f4b to 63c60d2 Compare September 25, 2023 12:59

abitrolly added 3 commits September 25, 2023 16:13

Use regexp interpolation instead of string concat

466055a

Move helper Pathname methods into Refinement

96ca1c5

https://docs.ruby-lang.org/en/2.4.0/syntax/refinements_rdoc.html

Skip backticks when printing env var name

7e835bc

Add proper sig for fail_with_message

cbee941

MikeMcQuaid reviewed Sep 25, 2023

View reviewed changes

abitrolly and others added 3 commits September 25, 2023 18:03

Update Library/Homebrew/dev-cmd/edit.rb

e3c0daf

Co-authored-by: Mike McQuaid <[email protected]>

Move condition in interpolated part into variable

6c7f3eb

Rename refinement

d16e7e1

Use any? to check for warning message and fix brew style

2445b68

Rename fail_with_message to raise_with_message!

10a55ec

MikeMcQuaid reviewed Sep 27, 2023

View reviewed changes

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

abitrolly added 2 commits September 27, 2023 12:39

Replace refinements with local methods

071a684

Fix typecheck errors

0c51aea

MikeMcQuaid approved these changes Sep 27, 2023

View reviewed changes

MikeMcQuaid merged commit 67be775 into Homebrew:master Sep 27, 2023
25 checks passed

abitrolly deleted the edit-simplify branch September 27, 2023 12:52

github-actions bot added the outdated PR was locked due to age label Oct 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev-cmd/edit: Move path specific functions to Pathname #16029

dev-cmd/edit: Move path specific functions to Pathname #16029

abitrolly commented Sep 24, 2023 •

edited

Loading

abitrolly commented Sep 24, 2023

apainintheneck commented Sep 24, 2023

abitrolly commented Sep 25, 2023

MikeMcQuaid left a comment

abitrolly commented Sep 25, 2023

MikeMcQuaid left a comment

MikeMcQuaid Sep 25, 2023

MikeMcQuaid Sep 26, 2023

abitrolly Sep 27, 2023

MikeMcQuaid Sep 25, 2023

abitrolly Sep 25, 2023

MikeMcQuaid Sep 26, 2023

apainintheneck commented Sep 26, 2023

abitrolly commented Sep 26, 2023

abitrolly commented Sep 26, 2023 •

edited

Loading

MikeMcQuaid commented Sep 26, 2023

dduugg commented Sep 26, 2023

abitrolly commented Sep 27, 2023 •

edited

Loading

MikeMcQuaid commented Sep 27, 2023

abitrolly commented Sep 27, 2023

MikeMcQuaid left a comment

abitrolly commented Sep 27, 2023

	def fail_with_message(path, cask)
	def raise_unless_existing!(path, cask)

dev-cmd/edit: Move path specific functions to Pathname #16029

dev-cmd/edit: Move path specific functions to Pathname #16029

Conversation

abitrolly commented Sep 24, 2023 • edited Loading

abitrolly commented Sep 24, 2023

apainintheneck commented Sep 24, 2023

abitrolly commented Sep 25, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

abitrolly commented Sep 25, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Sep 25, 2023

Choose a reason for hiding this comment

MikeMcQuaid Sep 26, 2023

Choose a reason for hiding this comment

abitrolly Sep 27, 2023

Choose a reason for hiding this comment

MikeMcQuaid Sep 25, 2023

Choose a reason for hiding this comment

abitrolly Sep 25, 2023

Choose a reason for hiding this comment

MikeMcQuaid Sep 26, 2023

Choose a reason for hiding this comment

apainintheneck commented Sep 26, 2023

abitrolly commented Sep 26, 2023

abitrolly commented Sep 26, 2023 • edited Loading

MikeMcQuaid commented Sep 26, 2023

dduugg commented Sep 26, 2023

abitrolly commented Sep 27, 2023 • edited Loading

MikeMcQuaid commented Sep 27, 2023

abitrolly commented Sep 27, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

abitrolly commented Sep 27, 2023

abitrolly commented Sep 24, 2023 •

edited

Loading

abitrolly commented Sep 26, 2023 •

edited

Loading

abitrolly commented Sep 27, 2023 •

edited

Loading