Extract CLI commands to individual classes #351

davidstosik · 2023-04-20T14:01:53Z

What are you trying to accomplish?

Packwerk's code around the CLI is currently hard to understand, mixes concepts together, and is overall very difficult to change. (Changing a seemingly simple thing in the CLI would sometimes requires editing many files).

This PR attempts a refactor of the code CLI in order to make it

easier to understand
easier to modify

What approach did you choose and why?

The first approach that we tried with @gmcgibbon involved using Thor, but we realized that using Thor while not breaking public APIs would be overly complicated, and end up trading one type of complexity for another.

In this PR, my approach was mainly to:

separate each CLI command to its own class
avoid introducing behaviour changes in public APIs (the CLI should behave the same, return same results and output same text, public classes should keep the same public methods with the same signature and expected behaviour)
make the CLI open-closed, allowing an external actor (such as a plugin?) to declare new CLI commands without having to monkey-patch any code in Packwerk.

What should reviewers focus on?

I know this looks rather big, but to this PR's defense, about 500 lines additions/deletions are test code lines that moved to a different file.
I left a few comments which you can check below about the things I'm unsure and would like a second opinion about.

If the approach sounds useful to you, you can also check the changes commit by commit, as I have made each commit a logical and justified change, ensuring that each commit also keeps the test suite green.

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

davidstosik · 2023-04-20T14:02:36Z

lib/packwerk.rb

-  autoload_under "commands" do
-    autoload :OffenseProgressMarker
-  end
-


This does not seem necessary, and was actually interfering with the autoload :Commands that we added above.

👍

Seems it was left behind in #124

davidstosik · 2023-04-20T14:10:15Z

lib/packwerk/cli.rb

-        true
-      when nil, "help"
-        usage
+      command = args.shift || "help"


If no command was passed, then default to help. Keeps the behaviour as before.

davidstosik · 2023-04-20T14:21:37Z

lib/packwerk/commands.rb

+      def class_for(command)
+        class_name = command.sub(" ", "_").underscore.classify + "Command"
+        if Commands.const_defined?(class_name)
+          Commands.const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings


I tried with the registry pattern first, but it required me to require all the files (with Dir[] and a glob for example) preemptively.
Using const_get, we can keep lazy loading.

I think const_get is ok, but we don't need this class_for method, just lean on autoloading to do this work for you. See my other comment. I ran tests and they all pass with the proposed change.

Yeah, so my idea was:

I wanted to extract the const_get + rescue to its own method to make the thing more readable (I didn't want to pollute Packwerk::Cli#execute_command with it). It first was a class method on Packwerk::Cli, and then naturally moved to Packwerk::Commands.

Once that was done, I noticed I was using exceptions as control flow whereas there is an appropriate method (#const_defined?) to check if the constant exists without raising NameError, so I used it instead.

I now realized that when I moved the method, I could have omitted the Commands here:

sig { params(command: String).returns(T.nilable(T.class_of(Commands::BaseCommand))) } def class_for(command) class_name = command.sub(" ", "_").underscore.classify + "Command" - if Commands.const_defined?(class_name) - Commands.const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings + if const_defined?(class_name) + const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings

I didn't want to pollute Packwerk::Cli#execute_command with it

Yeah but now we have a new public method to support, just for a single use case. Feels to me more appropriate just to hide the implementation in the only place that actually uses it (esp since it's just a couple lines).

I noticed I was using exceptions as control flow

Calling an undefined command is an "exception" though, so feels to me that rescuing here is fine (to avoid exposing an exception to the user). I would consider it "control flow" if we were then doing something other work in the rescue, but we're literally just cleaning up the output.

a new public method to support

That's a very fair point! Maybe I can move the method back to Cli, make it private, and switch back to rescuing NameError. I don't have a strong opinion on this bit.

davidstosik · 2023-04-20T14:27:39Z

lib/packwerk/commands/help_command.rb

+            init - set up packwerk
+            check - run all checks
+            update-todo - update package_todo.yml files
+            validate - verify integrity of packwerk and package configuration
+            version - output packwerk version
+            help  - display help information about packwerk


Because I kept lazy loading, and commands are not loaded, this help message cannot be generated dynamically.
I'm not super happy about it, does anyone have thoughts?

Maybe we could use the registry pattern just for help messages? So have some registry that commands can call to register their help message, but let the actual command class be autoloaded.

registry pattern

Yup! See my other comment with a link to a specific commit. That's what I used first. But that would mean we need to eager load CLI commands.

Not necessarily. There could be a registry that can be pre-populated for in-gem commands. For commands added externally, the process would be to register your extension (for the help message) and also create an autoloadable constant, which would not be loaded until the command was actually used.

Ahh, so instead of registering the commands from inside the command class, we could do it outside (in a place that we know gets loaded on init), like so?

Commands.register("update-todo", help: "update package_todo.yml files", aliases: ["update"])

For the gem's core commands, these would all be located in the same location, while plugin gems would have to do that somewhere, where they get loaded (eg. their own lib/packwerk/plugin_name.rb file).

#register could possibly also offer a way to have command and class name be different, like so:

Commands.register("something", help: "does something", classname: "MyCommandClass")

Yeah right exactly 👍

Additional commits:

Go back to using the registry pattern to register CLI command 386ffff

Generate the help command output based on registered commands d5c7dbe

Some autoload tests caught me loading more than I should:

(Didn't know about bin/rake test:loading.)

So I added one more commit: 721b197 (Move command registration to Cli in order to avoid early loading)

davidstosik · 2023-04-20T15:22:11Z

lib/packwerk/commands/uses_parse_run.rb

This module is a temporary to keep the behaviour as it was previously, focusing this PR on a refactor that mostly does not change behaviour.

However, the respective behaviours of check and update-todo are expected to diverge:

--offenses-formatter does not make much sense in the case of update-todo

update-todo does not accept a list of files (either passed as extra arguments or with --packages

When we work on this separation of behaviours, this module will naturally go away.

davidstosik · 2023-04-20T15:27:44Z

lib/packwerk/cli/result.rb

I removed this because it didn't seem to make sense anymore:

it's never returned by methods that are part of the public API

it imposes the commands to return their last message wrapped in a Result object, instead of outputting it in the relevant IO object that they know about, and to which they keep outputting during their execution

davidstosik · 2023-04-20T15:36:57Z

test/unit/packwerk/parse_run_test.rb

This test file, and how I had to move all its tests to CheckCommandTest and UpdateCommandTest, represents a +596 −505 diff all by itself, and is what grew this PR to its current +1,063 −777.

There are a few notable changes in each test though. Here's a single test's diff to illustrate those changes:

--- before.txt 2023-04-20 15:35:47.222809962 +0000 +++ after.txt 2023-04-20 15:36:31.176723735 +0000 @@ -1,4 +1,4 @@ - test "#check does not list stale violations when run on a single file with no violations, even if the containing package has violations" do + test "#run does not list stale violations when run on a single file with no violations, even if the containing package has violations" do use_template(:minimal) file_to_check = "components/source/some/path.rb" other_file = "components/source/some/other/path.rb" @@ -19,27 +19,31 @@ - #{other_file} YML + RunContext.any_instance.stubs(:process_file).returns([]) + FilesForProcessing.any_instance.stubs(:files).returns(Set.new([file_to_check])) + out = StringIO.new - parse_run = ParseRun.new( - relative_file_set: Set.new([file_to_check]), - configuration: Configuration.new({ "parallel" => false }), - progress_formatter: Formatters::ProgressFormatter.new(out) + configuration = Configuration.new({ "parallel" => false }) + check_command = CheckCommand.new( + [], + configuration: configuration, + out: out, + err_out: StringIO.new, + progress_formatter: Formatters::ProgressFormatter.new(out), + offenses_formatter: configuration.offenses_formatter ) - RunContext.any_instance.stubs(:process_file).returns([]) - result = parse_run.check + + result = check_command.run expected_output = <<~EOS 📦 Packwerk is inspecting 1 file \\. 📦 Finished in \\d+\\.\\d+ seconds - EOS - assert_match(/#{expected_output}/, out.string) - expected_message = <<~EOS No offenses detected No stale violations detected EOS - assert_equal expected_message, result.message + assert_match(/#{expected_output}/, out.string) - assert result.status + assert result end

davidstosik · 2023-04-20T15:37:56Z

lib/packwerk/parse_run.rb

I'm tempted to rename this file offenses_finder.rb now. 🤔
That's the only logic that's left in there.

davidstosik · 2023-04-27T10:38:18Z

lib/packwerk/commands/check_command.rb

+            Specify files or check the include and exclude glob in the config file.
+          MSG
+
+          true


It feels like it should return false if the above was an error, but this is the existing behaviour.

Also, we pass an err_out around but it feels like we're not using it as much as we should...

In the spirit of keeping this PR a low impact refactor, I decided not to change those.

davidstosik · 2023-04-27T10:42:16Z

lib/packwerk/commands/help_command.rb

+
+      sig { override.returns(T::Boolean) }
+      def run
+        @err_out.puts(<<~USAGE)


Here too, I'm preserving existing behaviour: this help message was output to err_out before, still is.

shioyama

Still reading through changes, but made some initial comments. Really great work! ❤️ Way cleaner and more extensible.

shioyama · 2023-05-01T01:09:40Z

lib/packwerk.rb

-  autoload_under "commands" do
-    autoload :OffenseProgressMarker
-  end
-


👍

Seems it was left behind in #124

shioyama · 2023-05-01T01:24:23Z

lib/packwerk/cli.rb

-      when nil, "help"
-        usage
+      command = args.shift || "help"
+      command_class = Commands.class_for(command)


You can avoid the need for class_for by just const_get-ting the command (converted to class name), and rescuing the possible NameError, like this:

class_name = command.sub(" ", "_").underscore.classify + "Command" Commands.const_get(class_name).new( args, configuration: @configuration, out: @out, err_out: @err_out, progress_formatter: @progress_formatter, offenses_formatter: @offenses_formatter, ).run rescue NameError @err_out.puts("'#{command}' is not a packwerk command. See `packwerk help`.",) false end

shioyama · 2023-05-01T01:26:05Z

lib/packwerk/commands.rb

+      def class_for(command)
+        class_name = command.sub(" ", "_").underscore.classify + "Command"
+        if Commands.const_defined?(class_name)
+          Commands.const_get(class_name) # rubocop:disable Sorbet/ConstantsFromStrings


I think const_get is ok, but we don't need this class_for method, just lean on autoloading to do this work for you. See my other comment. I ran tests and they all pass with the proposed change.

shioyama · 2023-05-01T01:28:51Z

lib/packwerk/commands/help_command.rb

+            init - set up packwerk
+            check - run all checks
+            update-todo - update package_todo.yml files
+            validate - verify integrity of packwerk and package configuration
+            version - output packwerk version
+            help  - display help information about packwerk


Maybe we could use the registry pattern just for help messages? So have some registry that commands can call to register their help message, but let the actual command class be autoloaded.

shioyama · 2023-05-01T01:32:13Z

lib/packwerk/commands.rb

+    autoload :CheckCommand
+    autoload :HelpCommand
+    autoload :InitCommand
+    autoload :UpdateCommand, "packwerk/commands/update_todo_command"


Maybe we should add some aliasing functionality to make this more natural and explicit? i.e. here we're aliasing update to update_todo, so rather than adding an autoload we could have execute_command look at a list of aliases and "redirect" the command accordingly if an alias exists.

Or do we also need to support the existence of the UpdateComand constant as well? 🤔

Or do we also need to support the existence of the UpdateCommand constant as well? 🤔

We don't. It's a class introduced in this PR, but doesn't have to.

My problem is as follows:

we use autoloading, so the class is not loaded until we have defined that it's necessary.

if we implement a system to declare aliases, then we need it to be run during initialization

I would like to avoid having to declare aliases outside of the command class itself (open-closed principle)

I guess the fact that we need these autoload is already an infringement to the open-closed principle though... 🤔

Check this commit to see an early idea I had, where each command was able to declare itself, and its aliases in the process. The caveat was that command classes needed to be eager loaded.

24d4033#diff-c990d324e3c6c514f831ceaa8414cb9450101d4f76e2ac32e80c812f2f455125R10

Note: this changes `packwerk help`'s output from stderr to stdout

Wondering if I should also deprecate #mark_as_inspected and

Note that I also moved and adjusted the corresponding tests to `CheckCommandTest`

…ng it

…all commands

… decide how/when/where to output messages

This time, registration happens from outside the command class, and using a classname string, so that we can keep the lazy loading behaviour.

davidstosik · 2023-05-02T09:48:31Z

lib/packwerk/cli.rb

+      extend T::Sig
+      sig { params(name: String, help: String, aliases: T::Array[String]).void }
+      def register_command(name, help:, aliases: [])
+        CommandRegistry.register(name, help: help, aliases: aliases)


This just delegates to CommandRegistry. It allows me to keep that class private, while it exposes public methods to be used within Packwerk.

gmcgibbon · 2023-05-02T18:54:09Z

lib/packwerk/cli.rb

  end
+
+  Cli.register_command("init", help: "set up packwerk")


The help text should be declared on the command class. It makes it easier to change and expand on.

See previous conversation with @shioyama : if I do so (which I'm not against), I'll need to either eager load command classes or at least lazy load them all when the user called for help.

In the case of the help command, I think it is fine to load all commands. Since this is done lazily I don't see a problem.

gmcgibbon · 2023-05-02T23:00:23Z

lib/packwerk/cli/command_registry.rb

+      sig { returns(T.class_of(Cli::BaseCommand)) }
+      def command_class
+        classname = @name.sub(" ", "_").underscore.classify + "Command"
+        Cli.const_get(classname) # rubocop:disable Sorbet/ConstantsFromStrings


Can you explain your motivation to not use a Packwerk::Cli::Commands module to const_get from? We could fit all command related code in one place (eg. Commands::Registry, Commands::Base, Commands::HelpCommand, etc. It seems safer and more organized.

Without the need to auto match a command name with a class name, and with instead an explicit registration of each command, I felt Commands had lost its main purpose, that triggered its creation: hosting all commands and nothing else so that packwerk not-a-command-but-const-with-matching-name-exists would fail beautifully and not weirdly.
With its main purpose lost, I went for the "less code is better" approach.

To me, commands are part of the CLI (command line interface 😬), so using Cli as namespace felt adequate.

Personally, I think the organization of the commands/ namespace makes command specific code easier to reason about, but I don't feel strongly about it. I'm slightly worried about this from a public API perspective.

Cli is public, sure, but if I make my constants nested under it private, it shouldn't make a difference?
I'm not antagonizing, just trying to understand where the problem would be exactly.

gmcgibbon · 2023-05-02T23:29:59Z

lib/packwerk/cli/check_command.rb

+        end
+
+        run_context = RunContext.from_configuration(@configuration)
+        offense_collection = OffenseCollection.new(@configuration.root_path)


These could be private methods.

Quite verbose, but possible: c8b3864

gmcgibbon · 2023-05-02T23:31:55Z

lib/packwerk/cli/uses_parse_run.rb

+      end
+
+      sig { returns(T::Hash[Symbol, T.untyped]) }
+      def parse_options


Thinking ahead slightly, it might make sense for the base class to implement def options; raise NotImplementedError; end (or Sorbet equivalent abstract class syntax) and have this be a memoized public method for commands to implement.

That'll come in a next PR where I split the options used by check and update-todo into separate option parsers. This PR felt already big enough.

I did not split in this PR too because splitting will also induce behavior change (I don't want update-todo you use --packages and --offenses-formatter anymore), and I'd like to make sure I get proper review on these bits.

gmcgibbon · 2023-05-02T23:36:46Z

lib/packwerk/cli/command_registry.rb

+
+module Packwerk
+  class Cli
+    class CommandRegistry


The instance of this class is an entry, not a registry, so it doesn't really make sense to me. I would say that a better api would be Commands.all / Commands.register, and then name this class Commands::Info, Commands::Lookup, or Commands::Entry (etc.) so the registry can live on Commands.

Yeah, the naming didn't please me. I'll take another look.

I renamed CommandRegistry to LazyLoadedCommand (to make the distinction with BaseCommand). d49a7ea

I prefer typing the registry as a T::Array[LazyLoadedCommand] rather than a T::Array[T::Hash[Symbol, T.untyped]] (where each value in the hash would look like { name: "foo", aliases: ["bar"] }).

I guess I could restore the Commands module/namespace anyway and have the registry live there... (But I'd rather each entry be a LazyLoadedCommand than a primitive.)

and then name this class Commands::Info, Commands::Lookup

Just understood what you meant here. Could work too. 🤔 Will try.

gmcgibbon · 2023-05-24T15:51:03Z

lib/packwerk/commands/check_command.rb

+        end
+
+        all_offenses = T.let([], T::Array[Offense])
+        on_interrupt = T.let(-> { @progress_formatter.interrupted }, T.proc.void)


Suggested change

on_interrupt = T.let(-> { @progress_formatter.interrupted }, T.proc.void)

on_interrupt = T.let(proc { @progress_formatter.interrupted }, T.proc.void)

These could also be memoized private methods

gmcgibbon · 2023-05-24T15:53:03Z

lib/packwerk/commands/lazy_loaded_entry.rb

+
+module Packwerk
+  module Commands
+    class LazyLoadedEntry


Suggested change

class LazyLoadedEntry

class Entry

Nit: I think LazyLoadedEntry is a little too verbose, but feel free to ignore this. I like the implementation!

gmcgibbon · 2023-05-24T15:55:49Z

lib/packwerk/commands/uses_parse_run.rb

+      end
+      def initialize(args, configuration:, out:, err_out:, progress_formatter:, offenses_formatter:)
+        @args = args
+        @configuration = configuration


These should be assigned by super, right?

Hmm, yeah, I think so, but there was a reason why I did that. Will get back to you when I remember. 😅

Ahh, yes, it's a Sorbet problem. If I don't make these assignments in this initialize method (ie if for example I comment these two lines), Sorbet will error with the following:

bin/srb tc 130 ↵ lib/packwerk/commands/uses_parse_run.rb:29: Use of undeclared variable @configuration https://srb.help/6002 29 | @configuration.parallel = @parsed_options[:parallel] ^^^^^^^^^^^^^^ Note: Use T.let to declare this variable. For more information, see https://sorbet.org/docs/type-annotations lib/packwerk/commands/uses_parse_run.rb:41: Use of undeclared variable @configuration https://srb.help/6002 41 | configuration: @configuration ^^^^^^^^^^^^^^ Note: Use T.let to declare this variable. For more information, see https://sorbet.org/docs/type-annotations lib/packwerk/commands/uses_parse_run.rb:56: Use of undeclared variable @configuration https://srb.help/6002 56 | parallel: @configuration.parallel?, ^^^^^^^^^^^^^^ Note: Use T.let to declare this variable. For more information, see https://sorbet.org/docs/type-annotations lib/packwerk/commands/uses_parse_run.rb:66: Use of undeclared variable @configuration https://srb.help/6002 66 | options[:parallel] = T.let(@configuration.parallel?, T::Boolean) ^^^^^^^^^^^^^^ Note: Use T.let to declare this variable. For more information, see https://sorbet.org/docs/type-annotations lib/packwerk/commands/uses_parse_run.rb:82: Use of undeclared variable @args https://srb.help/6002 82 | end.parse!(@args) ^^^^^ Note: Use T.let to declare this variable. For more information, see https://sorbet.org/docs/type-annotations Errors: 5

Here's a minimal reproduction on Sorbet.run:

# typed: strict class Foo extend T::Sig sig { params(param: String).void } def initialize(param) @param = param end end module Bar extend T::Sig extend T::Helpers requires_ancestor { Foo } sig { params(param: String).void } def initialize(param) # @param = param # This line fixes the Sorbet error on line 22 super @param << " concatenated" # Error "Use of undeclared variable @param" end end

I think using attribute readers as you suggested in another comment could allow us to avoid this repetition. 🤔

2d9c5c0#diff-7a35006173631f73820a5c2882b286cbe58356933b7e74a50c5446d1c1547ce9L25

Hm, I wonder if this is a bug in Sorbet. If we require the class as an ancestor, it should always expect the initializer to run.

True, I guess it can't know for sure. The workaround is good 👍

gmcgibbon · 2023-05-24T15:56:36Z

lib/packwerk/commands/base_command.rb

+        @out = out
+        @err_out = err_out
+        @progress_formatter = progress_formatter
+        @offenses_formatter = offenses_formatter


I'm interested in how you feel about using ivars for stdout/stderr/args/formatters? Should we create private/protected/public readers for these ivars? It seems like it would be a nicer public API to me.

Yeah, I like the idea. I tend to define accessors for everything usually. I did not do it here because Sorbet requires a level of verbosity I was not super comfortable with. I'll give it a try.

…ent names

… clarity

gmcgibbon · 2023-05-31T17:58:59Z

lib/packwerk/commands/uses_parse_run.rb

+      end
+      def initialize(args, configuration:, out:, err_out:, progress_formatter:, offenses_formatter:)
+        super
+        @_parsed_options = T.let(nil, T.nilable(T::Hash[Symbol, T.untyped]))


Suggested change

@_parsed_options = T.let(nil, T.nilable(T::Hash[Symbol, T.untyped]))

@parsed_options = T.let(nil, T.nilable(T::Hash[Symbol, T.untyped]))

I don't think this needs to be underscored. No other ivar is.

Ah, I used an underscore because it's a memoization ivar for the parsed_options method, and Sorbet seemed to require this T.let in the initializer. This might not be true though, I'll check.

gmcgibbon · 2023-05-31T18:00:00Z

lib/packwerk/formatters/progress_formatter.rb

@@ -44,6 +53,7 @@ def mark_as_failed
      def interrupted
        @out.puts
        @out.puts("Manually interrupted. Violations caught so far are listed below:")
+        @out.puts


Are these extra lines necessary, or just for readability?

They are not new, they were just moved from somewhere else to keep the tests happy.
If I recall correctly, it used to be here:

packwerk/lib/packwerk/cli.rb

Line 131 in f317dea

@out.puts

The change happened in this commit: 1bed3e8

…initializer

davidstosik force-pushed the sto/command-classes branch from 098c4d6 to a83f2bd Compare April 20, 2023 14:16

davidstosik commented Apr 20, 2023

View reviewed changes

meganemura mentioned this pull request Apr 20, 2023

Add command line option to toggle parallel execution #349

Merged

6 tasks

davidstosik commented Apr 20, 2023

View reviewed changes

davidstosik force-pushed the sto/command-classes branch from 2f07b42 to c2f99a8 Compare April 21, 2023 00:56

davidstosik mentioned this pull request Apr 26, 2023

Implement a count-offenses command #340

Draft

6 tasks

davidstosik commented Apr 27, 2023

View reviewed changes

davidstosik marked this pull request as ready for review April 27, 2023 11:05

davidstosik requested a review from a team as a code owner April 27, 2023 11:05

davidstosik changed the title ~~[WIP] Extract CLI commands to individual classes~~ Extract CLI commands to individual classes Apr 27, 2023

davidstosik requested a review from shioyama April 27, 2023 11:12

shioyama reviewed May 1, 2023

View reviewed changes

davidstosik added 12 commits May 2, 2023 08:03

Move Cli#usage to a HelpCommand class

26b4707

Move Cli#version to a VersionCommand class

aedd5ed

Move Cli#validate to a ValidateCommand class

06b229d

Move Cli#init to a InitCommand class

ef473c0

Move Cli's check to a CheckCommand class

9e976ec

Move Cli's update to an UpdateCommand class

8f5b0f3

Make Cli::InitCommand return a Cli::Result object

189932a

Make Cli::ValidateCommand return a Cli::Result object

16bd604

Make Cli::VersionCommand return a Cli::Result object

1bed3e8

Make Cli::HelpCommand return a Cli::Result object

fb880a6

Note: this changes `packwerk help`'s output from stderr to stdout

Stop passing out IO when unnecessary

676ea30

Factorize common parts out of case/when

9d2a966

davidstosik added 12 commits May 2, 2023 08:03

Move if failed logic to progress formatter

bcd5b78

Wondering if I should also deprecate #mark_as_inspected and

Instead of passing CLI to commands, only pass its necessary components

2298c43

Move packwerk check logic to Cli::CheckCommand

7044d1c

Note that I also moved and adjusted the corresponding tests to `CheckCommandTest`

Move packwerk update logic to Cli::UpdateCommand

688e80d

Simplify the ParseRun class so it only takes arguments it needs

450d5f5

Return 'no files found or given' as a Cli::Result instead of outputti…

d877db5

…ng it

Remove autoload for OffenseProgressMarker as the constant is gone

fc5389c

Use const_get instead of a command registry to avoid eager loading …

6eb3876

…all commands

Remove the concept of Cli::Result, and leave it to command classes to…

46c6fc3

… decide how/when/where to output messages

Use instance variables instead of private attribute readers

ccca6d6

Go back to using the registry pattern to register CLI command

386ffff

This time, registration happens from outside the command class, and using a classname string, so that we can keep the lazy loading behaviour.

Generate the help command output based on registered commands

d5c7dbe

davidstosik force-pushed the sto/command-classes branch from c2f99a8 to d5c7dbe Compare May 2, 2023 09:45

davidstosik commented May 2, 2023

View reviewed changes

Move command registration to Cli in order to avoid early loading

721b197

davidstosik force-pushed the sto/command-classes branch from 6e1f7ae to 721b197 Compare May 2, 2023 10:00

gmcgibbon reviewed May 2, 2023

View reviewed changes

davidstosik added 4 commits May 8, 2023 12:00

Define each command's description inside the command class

c127e09

Rename CommandRegistry to more accurate LazyLoadedCommand

d49a7ea

Restore the Commands namespace and make better use of it

670fc67

Extract private methods in CheckCommand

c8b3864

gmcgibbon reviewed May 24, 2023

View reviewed changes

davidstosik added 3 commits May 31, 2023 09:53

Use attr_readers instead of ivars

2d9c5c0

Memoize #parsed_options instead of having ivar and method with differ…

c5077ee

…ent names

Rename find_offenses_formatter to offenses_formatter_from_options for…

d29ee32

… clarity

gmcgibbon approved these changes May 31, 2023

View reviewed changes

Remove _ from memoization ivar and remove its T.let declaration from …

4ccb564

…initializer

davidstosik merged commit 0c678b0 into main Jun 2, 2023

davidstosik deleted the sto/command-classes branch June 2, 2023 02:40

shopify-shipit bot temporarily deployed to rubygems August 8, 2023 20:48 Inactive

	on_interrupt = T.let(-> { @progress_formatter.interrupted }, T.proc.void)
	on_interrupt = T.let(proc { @progress_formatter.interrupted }, T.proc.void)

	@_parsed_options = T.let(nil, T.nilable(T::Hash[Symbol, T.untyped]))
	@parsed_options = T.let(nil, T.nilable(T::Hash[Symbol, T.untyped]))

Extract CLI commands to individual classes #351

Extract CLI commands to individual classes #351

Conversation

davidstosik commented Apr 20, 2023 • edited Loading

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Checklist

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

davidstosik May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidstosik May 1, 2023 • edited Loading

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

davidstosik Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shioyama left a 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

davidstosik May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon May 2, 2023 • edited Loading

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

davidstosik May 5, 2023 • edited Loading

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

gmcgibbon May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon May 2, 2023 • edited Loading

Choose a reason for hiding this comment

davidstosik May 5, 2023 • edited Loading

Choose a reason for hiding this comment

davidstosik May 8, 2023 • edited Loading

Choose a reason for hiding this comment

davidstosik May 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidstosik May 31, 2023 • edited Loading

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

davidstosik commented Apr 20, 2023 •

edited

Loading

davidstosik May 1, 2023 •

edited

Loading

davidstosik May 1, 2023 •

edited

Loading

davidstosik Apr 20, 2023 •

edited

Loading

davidstosik May 1, 2023 •

edited

Loading

gmcgibbon May 2, 2023 •

edited

Loading

davidstosik May 5, 2023 •

edited

Loading

gmcgibbon May 2, 2023 •

edited

Loading

gmcgibbon May 2, 2023 •

edited

Loading

davidstosik May 5, 2023 •

edited

Loading

davidstosik May 8, 2023 •

edited

Loading

davidstosik May 8, 2023 •

edited

Loading

gmcgibbon May 24, 2023 •

edited

Loading

davidstosik May 31, 2023 •

edited

Loading