Enable strict typing in `CLI::Parser` #17030

dduugg · 2024-04-04T19:29:21Z

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?

I just wanted to capture #parser to work on Args namespacing, but decided to go all the way. (There's a pretty decent chance of type error creeping in, it's hard to find the right balance of rigor & narrowness without allowing an opportunity for some to slip in.)

dduugg · 2024-04-04T19:29:56Z

Library/Homebrew/abstract_command.rb

@@ -26,6 +26,9 @@ def command_name = Utils.underscore(T.must(name).split("::").fetch(-1)).tr("_",
      sig { params(name: String).returns(T.nilable(T.class_of(AbstractCommand))) }
      def command(name) = subclasses.find { _1.command_name == name }

+      sig { returns(T::Boolean) }
+      def dev_cmd? = T.must(name).start_with?("Homebrew::DevCmd")


This logic seems more appropriate to capture here

dduugg · 2024-04-04T19:30:23Z

Library/Homebrew/cli/parser.rb

@@ -9,15 +9,28 @@
 require "set"
 require "utils/tty"

-COMMAND_DESC_WIDTH = 80
-OPTION_DESC_WIDTH = 45
-HIDDEN_DESC_PLACEHOLDER = "@@HIDDEN@@"


I moved these out of top level while I was here

dduugg · 2024-04-04T19:31:04Z

Library/Homebrew/cli/parser.rb

@@ -119,49 +133,52 @@ def self.global_options
        ]
      end

+      sig { params(option: String).returns(String) }
+      def self.option_to_name(option)


I hoisted this with other class methods, to follow class layout conventions

There might be a cop for that.

Indeed, there is (though disabled by default)! https://docs.rubocop.org/rubocop/cops_layout.html#layoutclassstructure

dduugg · 2024-04-04T19:31:20Z

Library/Homebrew/cli/parser.rb

-        @parser.summary_indent = " " * 2
-
+        @parser = T.let(OptionParser.new, OptionParser)
+        @parser.summary_indent = "  "


It made more sense to just do the math on this one, IMO

dduugg · 2024-04-04T19:31:45Z

Library/Homebrew/cli/parser.rb

        else
          # FIXME: remove once commands are all subclasses of `AbstractCommand`:
          # Filter out Sorbet runtime type checking method calls.
-          cmd_location = T.must(caller_locations).select do |location|
+          cmd_location = caller_locations.select do |location|


Not really sure why the T.must is no longer necessary, but \o/

dduugg · 2024-04-04T19:34:13Z

Library/Homebrew/cli/parser.rb

          @min_named_args = @max_named_args = number
-        elsif min.present? || max.present?
+        elsif min || max


removed the unecessary present?s while i was here

dduugg · 2024-04-04T19:34:36Z

Library/Homebrew/cli/parser.rb

@@ -459,11 +479,7 @@ def hide_from_man_page!

      private

-      SYMBOL_TO_USAGE_MAPPING = {


this isn't actually private, i hoisted it and added a private_constant declaration

dduugg · 2024-04-04T19:35:06Z

Library/Homebrew/cli/parser.rb

@@ -590,12 +614,13 @@ def check_conflicts
          end

          select_cli_arg = violations.count - env_var_options.count == 1
-          raise OptionConflictError, violations.map(&method(:name_to_option)) unless select_cli_arg
+          raise OptionConflictError, violations.map { name_to_option(_1) } unless select_cli_arg


sorbet requires these to be removed under strict typing, to validate method arity

dduugg · 2024-04-04T19:35:48Z

Library/Homebrew/cli/parser.rb

@@ -282,17 +294,23 @@ def name_to_option(name)
        end
      end

+      sig { params(names: String).returns(T.nilable(String)) }
      def option_to_description(*names)
        names.map { |name| name.to_s.sub(/\A--?/, "").tr("-", " ") }.max


i think this logic is wrong (pretty sure it's choosing alphabetically, which seems off), but i left it alone

I agree. It's only used in the #option_description method as a fallback if an explicit description doesn't exist. It'd probably make more sense to grab the max by string length instead. I think all of our internal commands have descriptions so it doesn't really matter much for us. I'm fine with fixing it here or as a follow-up though.

dduugg · 2024-04-04T19:36:40Z

Library/Homebrew/utils/formatter.rb

@@ -7,6 +7,9 @@
 #
 # @api private
 module Formatter
+  COMMAND_DESC_WIDTH = 80
+  OPTION_DESC_WIDTH = 45


These are only used in calls to Formatter, so seemed to make the most sense here

MikeMcQuaid

Great work here @dduugg! A few nits but happy for them to be addressed in follow-up PRs or inline and then self-merged.

Library/Homebrew/brew.rb

Library/Homebrew/cli/parser.rb

MikeMcQuaid · 2024-04-05T07:41:00Z

Library/Homebrew/cli/parser.rb

+      sig { params(text: T.nilable(String)).returns(T.nilable(String)) }
      def description(text = nil)
        return @description if text.blank?


Doesn't need changed now but: this type of method API kinda sucks with Sorbet to me. Better to fall back to getters/setters.

Similarly, I wonder about returning empty strings more often rather than nilable strings.

Library/Homebrew/cli/parser.rb

MikeMcQuaid

@dduugg One final "add a Ruby comment" nit and I'm ✅ to ship!

Library/Homebrew/cli/parser.rb

apainintheneck · 2024-04-16T03:05:38Z

Library/Homebrew/cli/parser.rb

@@ -119,49 +133,52 @@ def self.global_options
        ]
      end

+      sig { params(option: String).returns(String) }
+      def self.option_to_name(option)


There might be a cop for that.

apainintheneck · 2024-04-16T03:21:45Z

Library/Homebrew/cli/parser.rb

@@ -282,17 +294,23 @@ def name_to_option(name)
        end
      end

+      sig { params(names: String).returns(T.nilable(String)) }
      def option_to_description(*names)
        names.map { |name| name.to_s.sub(/\A--?/, "").tr("-", " ") }.max


I agree. It's only used in the #option_description method as a fallback if an explicit description doesn't exist. It'd probably make more sense to grab the max by string length instead. I think all of our internal commands have descriptions so it doesn't really matter much for us. I'm fine with fixing it here or as a follow-up though.

MikeMcQuaid

@dduugg Good to self-merge when you are!

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

dduugg commented Apr 4, 2024

View reviewed changes

dduugg force-pushed the strict-parser branch 3 times, most recently from 6a96f88 to 46549dd Compare April 4, 2024 20:51

MikeMcQuaid approved these changes Apr 5, 2024

View reviewed changes

dduugg mentioned this pull request Apr 8, 2024

Prefer numbered block params over proc conversion, cont'd #17047

Merged

7 tasks

MikeMcQuaid approved these changes Apr 12, 2024

View reviewed changes

Library/Homebrew/cli/parser.rb Show resolved Hide resolved

apainintheneck approved these changes Apr 16, 2024

View reviewed changes

MikeMcQuaid approved these changes Apr 16, 2024

View reviewed changes

dduugg and others added 4 commits April 21, 2024 14:16

Enable strict typing in CLI::Parser

cb71938

Apply suggestions from code review

98d36c3

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

cli/parser: improve mutable constant comment.

ed6f306

cleanup

1172013

dduugg force-pushed the strict-parser branch from 65a4c9a to 1172013 Compare April 21, 2024 21:35

dduugg enabled auto-merge April 21, 2024 21:35

dduugg merged commit cfa3f92 into master Apr 21, 2024
30 checks passed

dduugg deleted the strict-parser branch April 21, 2024 21:49

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable strict typing in `CLI::Parser` #17030

Enable strict typing in `CLI::Parser` #17030

dduugg commented Apr 4, 2024 •

edited

Loading

dduugg Apr 4, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

apainintheneck Apr 16, 2024

dduugg Apr 21, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

dduugg Apr 4, 2024

apainintheneck Apr 16, 2024

dduugg Apr 4, 2024

MikeMcQuaid left a comment

MikeMcQuaid Apr 5, 2024

MikeMcQuaid left a comment

apainintheneck Apr 16, 2024

apainintheneck Apr 16, 2024

MikeMcQuaid left a comment

		@@ -459,11 +479,7 @@ def hide_from_man_page!

		private

		SYMBOL_TO_USAGE_MAPPING = {

Enable strict typing in CLI::Parser #17030

Enable strict typing in CLI::Parser #17030

Conversation

dduugg commented Apr 4, 2024 • 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

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

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Enable strict typing in `CLI::Parser` #17030

Enable strict typing in `CLI::Parser` #17030

dduugg commented Apr 4, 2024 •

edited

Loading