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

Emit warnings for mistyped options #1860

Closed
MikeMcQuaid opened this Issue Jan 16, 2017 · 13 comments

Comments

Projects
None yet
7 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Jan 16, 2017

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).

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Jan 25, 2017

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member Author

MikeMcQuaid commented Jan 25, 2017

@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.

@Qchmqs

This comment has been minimized.

Copy link

Qchmqs commented Jan 29, 2017

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member Author

MikeMcQuaid commented Jan 30, 2017

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

@MikeMcQuaid MikeMcQuaid referenced this issue Feb 24, 2017

Closed

invalid flags silently accepted #2169

3 of 3 tasks complete
@lwe

This comment has been minimized.

Copy link
Contributor

lwe commented Mar 12, 2017

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.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member Author

MikeMcQuaid commented Mar 14, 2017

@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.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 19, 2017

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

@MikeMcQuaid MikeMcQuaid added in progress and removed help wanted labels Jun 19, 2017

@RandomDSdevel

This comment has been minimized.

Copy link
Contributor

RandomDSdevel commented Jun 19, 2017

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

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Jun 20, 2017

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

@RandomDSdevel

This comment has been minimized.

Copy link
Contributor

RandomDSdevel commented Jun 20, 2017

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

@GauthamGoli GauthamGoli referenced this issue Dec 11, 2018

Merged

cleanup: Use CLI::Parser to parse args #5395

5 of 6 tasks complete

@GauthamGoli GauthamGoli referenced this issue Jan 29, 2019

Merged

cmd/upgrade: Use CLI::Parser to parse args #5643

3 of 6 tasks complete

@MikeMcQuaid MikeMcQuaid referenced this issue Jan 30, 2019

Merged

Finish option handling #5650

6 of 6 tasks complete
@MikeMcQuaid

This comment has been minimized.

Copy link
Member Author

MikeMcQuaid commented Jan 31, 2019

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

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Jan 31, 2019

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.

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Feb 2, 2019

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!

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