Convert next batch of dev commands to use AbstractCommand #16937

dduugg · 2024-03-22T01:02:58Z

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?

Ports the next 15 dev commands to use AbstractCommand. See prior art in #16921

dduugg · 2024-03-22T01:05:44Z

Library/Homebrew/dev-cmd/formula.rb

+  module DevCmd
+    class FormulaCmd < AbstractCommand
+      sig { returns(String) }
+      def self.command_name = "formula"


For this command, as well as livecheck, naming the class in the usual manner (i.e. Formula and Livecheck) causes a bunch of type resolution errors in the command directories. Rather than resolve those to use fully-qualified paths, I gave the classes different names, and overrode their command_name values so that the command still works in the usual way.

I wonder if it's worth automatically stripping -cmd suffixes from the generated command_name so we don't need to do this every time. (And we can use self.command_name= in case we actually want to keep -cmd).

Good call, I add automagical suffix deletion in the latest commit.

carlocab · 2024-03-22T02:08:25Z

Library/Homebrew/dev-cmd/edit.rb

+               !Homebrew::EnvConfig.no_install_from_api? &&
+               !Homebrew::EnvConfig.no_env_hints? &&


Not your addition, but it seems nicer to hoist these conditions out of the loop so we don't need to recheck it for each path.

Agreed, but would prefer to scope that to another PR

carlocab · 2024-03-22T02:10:41Z

Library/Homebrew/dev-cmd/formula.rb

+  module DevCmd
+    class FormulaCmd < AbstractCommand
+      sig { returns(String) }
+      def self.command_name = "formula"


I wonder if it's worth automatically stripping -cmd suffixes from the generated command_name so we don't need to do this every time. (And we can use self.command_name= in case we actually want to keep -cmd).

carlocab · 2024-03-22T02:14:24Z

Library/Homebrew/dev-cmd/irb.rb

+
+      # work around IRB modifying ARGV.
+      sig { params(argv: T.nilable(T::Array[String])).void }
+      def initialize(argv = nil) = super(argv || ARGV.dup.freeze)


Didn't realise you could do this (i.e. def foo(x) = bar(x)) now.

carlocab · 2024-03-22T02:16:11Z

Library/Homebrew/test/dev-cmd/create_spec.rb

  let(:url) { "file://#{TEST_FIXTURE_DIR}/tarballs/testball-0.1.tbz" }
  let(:formula_file) { CoreTap.instance.new_formula_path("testball") }

-  it_behaves_like "parseable arguments"
+  it_behaves_like "parseable arguments", argv: ["foo"]


It seems weird to need to pass a garbage argv all over the place.

Good point. I've updated the helper to create a suitable argv by inspecting the parser's @min_named_args ivar.

carlocab · 2024-03-22T02:16:31Z

Library/Homebrew/test/dev-cmd/dispatch-build-bottle_spec.rb

-RSpec.describe "brew dispatch-build-bottle" do
-  it_behaves_like "parseable arguments"
+RSpec.describe Homebrew::DevCmd::DispatchBuildBottle do
+  it_behaves_like "parseable arguments", argv: ["foo"]


Here too, re argv.

carlocab · 2024-03-22T02:18:51Z

Library/Homebrew/test/dev-cmd/extract_spec.rb

-RSpec.describe "brew extract" do
-  it_behaves_like "parseable arguments"
+RSpec.describe Homebrew::DevCmd::Extract do
+  it_behaves_like "parseable arguments", argv: ["foo", "bar"]


Maybe we should just be able to pass the minimum number of required arguments instead of having to pass a garbage array every time?

carlocab · 2024-03-22T02:19:06Z

Library/Homebrew/test/dev-cmd/formula_spec.rb

-RSpec.describe "brew formula" do
-  it_behaves_like "parseable arguments"
+RSpec.describe Homebrew::DevCmd::FormulaCmd do
+  it_behaves_like "parseable arguments", argv: ["foo"]


Ditto re argv.

carlocab · 2024-03-22T02:19:33Z

Library/Homebrew/test/dev-cmd/pr-publish_spec.rb

-RSpec.describe "brew pr-publish" do
-  it_behaves_like "parseable arguments"
+RSpec.describe Homebrew::DevCmd::PrPublish do
+  it_behaves_like "parseable arguments", argv: ["foo"]


Garbage argv again.

dduugg · 2024-03-22T02:55:47Z

Great feedback, @carlocab, I super duper appreciate the time spent reviewing PRs like this. PTAL.

carlocab

LGTM, thanks @dduugg!

MikeMcQuaid

Great work here @dduugg and thanks for the insightful comments @carlocab!

MikeMcQuaid · 2024-03-22T08:35:23Z

Library/Homebrew/test/dev-cmd/install-bundler-gems_spec.rb

+require "cmd/shared_examples/args_parse"
+require "dev-cmd/install-bundler-gems"
+
+RSpec.describe Homebrew::DevCmd::InstallBundlerGems do


Meta: might be nice to have a test some time for ensuring each command/dev-cmd has a test?

dduugg commented Mar 22, 2024

View reviewed changes

dduugg changed the title ~~Ported cmds~~ Convert next batch of dev commands to use AbstractCommand Mar 22, 2024

dduugg added 16 commits March 21, 2024 19:04

Port Homebrew::DevCmd::Create

9297a85

Port Homebrew::DevCmd::DetermineTestRunners

19f9c52

Port Homebrew::DevCmd::DispatchBuildBottle

21c8455

Port Homebrew::DevCmd::Edit

2cc7054

Port Homebrew::DevCmd::Extract

2f461b1

Port Homebrew::DevCmd::Formula

16b93c4

Port Homebrew::DevCmd::GenerateCaskApi

afceaec

Port Homebrew::DevCmd::GenerateFormulaApi

7fc2064

Port Homebrew::DevCmd::GenerateManCompletions

485574a

Port Homebrew::DevCmd::InstallBundlerGems

0e489a8

Port Homebrew::DevCmd::Irb

afaa48b

Port Homebrew::DevCmd::Linkage

5848c3d

Port Homebrew::DevCmd::Livecheck

d15f995

Port Homebrew::DevCmd::PrAutomerge

972e853

Port Homebrew::DevCmd::PrPublish

b37274d

Fix test

26b3f30

dduugg force-pushed the ported-cmds branch from b49cf56 to 26b3f30 Compare March 22, 2024 02:05

carlocab approved these changes Mar 22, 2024

View reviewed changes

dduugg added 2 commits March 21, 2024 19:48

Use @min_named_args to generate argv

97370b4

Automagically remove -cmd suffix

821f2a5

dduugg force-pushed the ported-cmds branch from e08c09f to 821f2a5 Compare March 22, 2024 02:55

carlocab approved these changes Mar 22, 2024

View reviewed changes

apainintheneck approved these changes Mar 22, 2024

View reviewed changes

MikeMcQuaid approved these changes Mar 22, 2024

View reviewed changes

MikeMcQuaid merged commit 999ecf8 into master Mar 22, 2024
25 checks passed

MikeMcQuaid deleted the ported-cmds branch March 22, 2024 08:36

dduugg mentioned this pull request Mar 22, 2024

Convert remaining dev commands to use AbstractCommand #16938

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label Apr 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert next batch of dev commands to use AbstractCommand #16937

Convert next batch of dev commands to use AbstractCommand #16937

dduugg commented Mar 22, 2024 •

edited

dduugg Mar 22, 2024

carlocab Mar 22, 2024

dduugg Mar 22, 2024

carlocab Mar 22, 2024

dduugg Mar 22, 2024

carlocab Mar 22, 2024

carlocab Mar 22, 2024

carlocab Mar 22, 2024

dduugg Mar 22, 2024

carlocab Mar 22, 2024

carlocab Mar 22, 2024

carlocab Mar 22, 2024

carlocab Mar 22, 2024

dduugg commented Mar 22, 2024

carlocab left a comment

MikeMcQuaid left a comment

MikeMcQuaid Mar 22, 2024

		!Homebrew::EnvConfig.no_install_from_api? &&
		!Homebrew::EnvConfig.no_env_hints? &&

Convert next batch of dev commands to use AbstractCommand #16937

Convert next batch of dev commands to use AbstractCommand #16937

Conversation

dduugg commented Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Mar 22, 2024

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Mar 22, 2024 •

edited