Brew: Emit warnings for mistyped options

Created on 16 Jan 2017  ·  13Comments  ·  Source: Homebrew/brew

e.g. brew cleanup --hlep should refuse to run rather than silently ignoring the --hlep argument. This will likely require implementing a new DSL for Homebrew commands that lists all the possible arguments (where currently just ARGV.include? is used).

features in progress outdated

Most helpful comment

Popping in from Homebrew retirement since I was still subscribed to the issue to say: congratulations @GauthamGoli – what an amazing achievement. I didn't think this issue would ever be solved, and it looks like it did indeed take a huge amount of work. Wonderful stuff.

All 13 comments

Why can't we use this, instead of implementing a new DSL for parsing options?
https://docs.ruby-lang.org/en/2.1.0/OptionParser.html

@GauthamGoli We could use that but it wouldn't solve the problem by itself. The issue is that ARGV.include? is used throughout the codebase as global state. This is solveable with or without a new option parser.

the way it is now, those are hardcoded, can we extract them into a yaml, and try to apply fuzzy search first ? i feel like this will prove slow

@Qchmqs Rather than a YAML we'd use a command DSL but yes, this might enable fuzzy search.

Another simple solution could be to create an ARGV.whitelist!(%w{--foobar --help}) in the commands that checks all args and raises on unknown arguments found. This approach would of course not work for install, but for other commands it would work very well. Biggest downside being that there is a large disconnect between actual place where the arg is used (could be anywhere in any file) vs. where the _known arguments_ are enforced. It still relies on globals and starts adding more dependency to ARGV.

A command/arg DSL would of course solve this problem, but this would be a rather large refactoring and there are about 500 call sites that use that global variable ARGV. First I think it would require changes that ARGV is passed as argument, rather than used as a global variable, before a command DSL can be implemented.

@lwe Agreed that the command/arg DSL would be the longer-term solution; I think that's probably the best thing to push towards at this point.

There's no work-in-progress on this at #2420.

@apjanke: Um, don't you mean something more along the lines of, "There's _some_ _now_/_new_ work in progress on this at #2420."…?

Uh, yes. I meant "now". Sorry.

@apjanke: Don't worry, it's cool. We all make typographical mistakes at some time or another. (Now, back to lurking…)

This is done 🎉! Thanks to @GauthamGoli for the vast majority of the hard work here and designing such great APIs to use 👏

Popping in from Homebrew retirement since I was still subscribed to the issue to say: congratulations @GauthamGoli – what an amazing achievement. I didn't think this issue would ever be solved, and it looks like it did indeed take a huge amount of work. Wonderful stuff.

Hear, hear! This is a big win, IMHO. Congratulations @GauthamGoli!

I've tried out the new changes and it's already caught me out for inadvertently using a bogus option on one of my own formulae. Thanks for getting this in!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

stejmurphy picture stejmurphy  ·  4Comments

fxcoudert picture fxcoudert  ·  3Comments

JustinTArthur picture JustinTArthur  ·  3Comments

hktalent picture hktalent  ·  4Comments

vitahlin picture vitahlin  ·  4Comments