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.

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

list.rb: use CLI::Parser and improve handling of flags #4067

Merged
merged 3 commits into from May 28, 2018

Conversation

Projects
None yet
6 participants
@maxim-belkin
Copy link
Contributor

maxim-belkin commented Apr 13, 2018

  • 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 tests with your changes locally?

brew list:

  • When --multiple is specified without --versions it results in the
    execution of the ls command with --multiple flag, which results in a
    failure. Here we assume that --versions flag was forgotten when a lone--multiple option is provided.

  • Use OpenStruct to handle arguments for list

Note: alternative solution to setting mode.versions? to true (line 39) would be to odie instead of opoo on line 38.

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Apr 13, 2018

Note: alternative solution to setting mode.versions? to true (line 39) would be to odie instead of opoo on line 38.

👍 on odie -- that send is a little confusing.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 13, 2018

well, in that case it (send) will go away! 👍

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 14, 2018

@maxim-belkin Please use Homebrew::CLI::Parser for parsing args, it uses OpenStruct

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 14, 2018

Please use Homebrew::CLI::Parser for parsing args, it uses OpenStruct

Agreed 👍

full_names = Formula.installed.map(&:full_name).sort(&tap_and_name_comparison)
return if full_names.empty?
puts Formatter.columns(full_names)
else
ENV["CLICOLOR"] = nil
exec "ls", *ARGV.options_only << HOMEBREW_CELLAR
end
elsif ARGV.verbose? || !$stdout.tty?
elsif args.verbose? || !$stdout.tty?

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 15, 2018

Member

Homebrew.args.verbose?

This comment has been minimized.

@maxim-belkin

maxim-belkin Apr 15, 2018

Contributor

what is the purpose of adding Homebrew. ?

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 16, 2018

Member

Options like verbose, force, quiet and debug are project wide, hence they are stored as global vars to be accessible everywhere.

If we have --verbose passed to a command and that calls method defined in some other file, it might also need --verbose and we don't want to be passing around them as parameters if possible.

if ARGV.include?("--pinned") || ARGV.include?("--versions")
filtered_list
if args.pinned? || args.versions?
filtered_list(args)

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 15, 2018

Member

Could make args -> @args, instance variable, then its not needed to pass it as an argument.


module Homebrew
module_function

def list
args = Homebrew::CLI::Parser.parse do

This comment has been minimized.

@reitermarkus

reitermarkus Apr 15, 2018

Member

No need for the Homebrew:: prefix, since this is already inside it.

@maxim-belkin maxim-belkin changed the title list.rb: fix option handling and use OpenStruct list.rb: use Homebrew::CLI::Parser and improved handling of flags Apr 15, 2018

@maxim-belkin maxim-belkin changed the title list.rb: use Homebrew::CLI::Parser and improved handling of flags list.rb: use CLI::Parser and improve handling of flags Apr 16, 2018

switch :verbose
end

if args.multiple? && !args.versions?

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 16, 2018

Member

You'd have to use @args everywhere else

full_names = Formula.installed.map(&:full_name).sort(&tap_and_name_comparison)
return if full_names.empty?
puts Formatter.columns(full_names)
else
ENV["CLICOLOR"] = nil
exec "ls", *ARGV.options_only << HOMEBREW_CELLAR
end
elsif ARGV.verbose? || !$stdout.tty?
elsif Homebrew.@args.verbose? || !$stdout.tty?

This comment has been minimized.

@GauthamGoli

GauthamGoli Apr 16, 2018

Member

This will still be Homebrew.args.verbose?

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 16, 2018

Looks good. Can you please squash the commits into a single/couple commit(s) ?

@maxim-belkin maxim-belkin force-pushed the maxim-belkin:fix-multiple branch from f7c8957 to adb1064 Apr 16, 2018

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

Is opoo message fine? Or should I change it to odie (and remove the line after it)?

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Apr 16, 2018

I'm still 👍 on making it odie, but will defer to @GauthamGoli on it 🙂

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 16, 2018

brew list can take other undeclared arguments like -t which would be passed to ls, there's bigger problem here, it's because of the option parser itself. It never accepts undeclared options and passing some undeclared option causes it to output invalid option error.

I guess, until there's solution for this, we should keep this PR on hold.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

does cli_parser allow something like flag "*"?

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

Looks like flag "-[ABCFGHLOPRSTUWabcdefghiklmnopqrstuwx1]*" works, no?

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 16, 2018

We'd prefer not to declare options of ls inside brew

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

makes sense

I guess the solution is to define acceptable LS_FLAGS (different on Mac and Linux) and use them here.

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 16, 2018

Better solution is to add an optional feature to Parser to not fail when undeclared options are passed, rather store them separately.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

  1. I think making Parser not fail for undeclared options is one part of the problem, whereas limiting acceptable (undeclared) flags for every command that accepts such flags is another.

  2. In the current implementation, it is not possible to pass flags that Parser knows: -f, -v, -q, or -d. How about --lsflags switch that would pass all the remaining args to ls?

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 16, 2018

Point 1 is solved, I have a working implementation for making Parser not fail for undeclared options.

I am yet to find a way to solve point number 2 and I think its inherent to OptionParser hence difficult(edit: not possible?) to override.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 16, 2018

I haven't seen your implementation, but make sure it doesn't let users pass ; /bin/rm -rf / 2>/dev/null as an option to ls :)

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 17, 2018

User can always specify ; rm -rf / the shell itself would interpret it as a command and we wouldn't have control over it. So its not command injection. Also, as we are not offering brew as a Service on a server, we can assume that user knows that those commands would run on his/her own machine :)

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 22, 2018

Here’s snippet from OptionParser’s source

begin
  sw, = complete(:short, opt)
  # short option matched.
  val = arg.sub(/\A-/, '')
  has_arg = true
rescue InvalidOption
  # if no short options match, try completion with long
  # options.
  sw, = complete(:long, opt)
  eq ||= !rest
end

The above code makes undeclared short options (Ex: -f ) to match with long options (Ex: --filename ) instead of raising InvalidOption , as a result leads to some issues. Couple of issues mentioned by @maxim-belkin #4067 (comment)

None of the OptionParsers from other standard libraries have this behaviour (Even ruby’s Getoptlong, and python’s argparse )

I have also raised a discussion with OptionParsers maintainer here https://bugs.ruby-lang.org/issues/14692 , asking him if its possible for a workaround/feature implementation to suppress this behavior.

There a no way to suppress this default behaviour, I think.

  • I tried implementing this functionality outside OptionParser but there are many edge cases, makes the code hard to read/maintain. Basically it is almost like writing OptionParser's logic.
  • We could use Getoptlong instead of OptionParser but would lose(miss?) a lot of "off-the-shelf" functionality provided by OptionParser which we might want to use in future.
  • Don't use Homebrew::CLI::Parser for brew list until there's a work around.

What’s the best way forward / any suggestions anyone?
cc @MikeMcQuaid @alyssais

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 22, 2018

yikes.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Apr 22, 2018

You could use a refinement to monkey-patch that part of OptionParser, but you'd have to copy-paste the whole parse_in_order method since the relevant part is deep inside that method.

We could use Getoptlong instead of OptionParser but would lose(miss?) a lot of "off-the-shelf" functionality provided by OptionParser which we might want to use in future.

Which “off-the-shelf” functionality does OptionParser provide?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 22, 2018

Point 1 is solved, I have a working implementation for making Parser not fail for undeclared options.

This should be something that is not on by default.

I am yet to find a way to solve point number 2 and I think its inherent to OptionParser hence difficult(edit: not possible?) to override.

I think this is fine. I don't think it's essential to be able to pass those options through to ls and I think brew handling those options consistently is better.

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 22, 2018

Yep, monkey patching would work for now, but we'd have to be mindful of changes in that library whenever we change Ruby version.

Which “off-the-shelf” functionality does OptionParser provide?

Data types support for arguments , Documentation/help text generation.
We'd have to implement these ourselves, meaning more maintenance

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 21, 2018

@MikeMcQuaid My bad. I'll get back on this today evening.

switch :verbose
end

if @args.multiple? && !@args.versions?

This comment has been minimized.

@GauthamGoli

GauthamGoli May 21, 2018

Member

This if block can be removed.
Specifying constraints on options is now possible. You can have a look at cli_parser_spec.rb to look at corresponding test cases.

This comment has been minimized.

@maxim-belkin

maxim-belkin May 21, 2018

Contributor

do I need to add anything here? If so, what is the difference between required_for and depends_on (except for expressing opposite direction of a relationship) ?

switch "--pinned"
switch "--versions"
switch "--full-name"
switch "--multiple"

This comment has been minimized.

@GauthamGoli

GauthamGoli May 21, 2018

Member

switch "--multiple", required_for: "--versions"

This comment has been minimized.

@reitermarkus

reitermarkus May 21, 2018

Member

Don't you mean depends_on? --multiple is optional.

This comment has been minimized.

@GauthamGoli

GauthamGoli May 22, 2018

Member

Ops.. I got that wrong, you're right.

switch "--pinned"
switch "--versions"
switch "--full-name"
switch "--multiple", required_for: "--versions"

This comment has been minimized.

@reitermarkus

reitermarkus May 21, 2018

Member

Should be depends_on.

This comment has been minimized.

@maxim-belkin

maxim-belkin May 21, 2018

Contributor

So, what does the phrase

`--multiple` requires `--versions`

in plain English correspond to in this new syntax?

switch "--multiple", depends_on: "--versions"

?

This comment has been minimized.

@reitermarkus

reitermarkus May 21, 2018

Member
switch "--multiple", depends_on: "--versions"

--multiple is optional and can only be given as a “sub-option” for --versions.

switch "--multiple", required_for: "--versions"

When --versions is given, --multiple also has to be specified.

@maxim-belkin maxim-belkin force-pushed the maxim-belkin:fix-multiple branch from f80d5fc to 7e9689d May 22, 2018

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented May 22, 2018

rebased and force-pushed

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 23, 2018

@maxim-belkin Sorry. I added constraints for flags but not switches. I'll get that done very soon, and then we could proceed with this.

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 25, 2018

@maxim-belkin Please rebase with master

Comment has been addressed.

maxim-belkin added some commits Apr 13, 2018

list.rb: use CLI::Parser to better handle args
* --multiple now assumes --versions
* Use CLI::Parser (that, in turns, uses OpenStruct) to handle arguments

@maxim-belkin maxim-belkin force-pushed the maxim-belkin:fix-multiple branch from 7e9689d to f7e308a May 25, 2018

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented May 27, 2018

I have tested this locally. Will be merged when @MikeMcQuaid gives 👍

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 28, 2018

Thanks @maxim-belkin! If people go 😭 over the death of custom flags we'll think about fiddling with this more but I'm not sure it's worth blocking this PR over.

@MikeMcQuaid MikeMcQuaid merged commit 67db701 into Homebrew:master May 28, 2018

1 of 3 checks passed

codecov/patch 0% of diff hit (target 69.87%)
Details
codecov/project 69.84% (-0.04%) compared to 67a1235
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label May 28, 2018

@maxim-belkin maxim-belkin deleted the maxim-belkin:fix-multiple branch May 28, 2018

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented May 29, 2018

This broke brew list -1 which now just errors out. See Homebrew/homebrew-core#28429

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented May 29, 2018

-1 is not necessary when piping output to another command.

Regardless, old brew list -1 is analogous to brew list | cat

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 29, 2018

-1 is not necessary when piping output to another command.

Regardless, given it's used by others, we should fix this.

@ilovezfs ilovezfs referenced this pull request May 29, 2018

Closed

Archey: fix bad `homebrew list` syntax #28429

4 of 4 tasks complete

@lock lock bot added the outdated label Jun 28, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jun 28, 2018

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