This is an archive of the discontinued Mercurial Phabricator instance.

fancyopts: add support for custom multi-arg opts in fancyopts.py
ClosedPublic

Authored by dploch on Feb 7 2018, 8:38 PM.

Details

Summary

This allows for more complex multi-arg opt logic, such as "--sum 1 --sum 2" -> 3, "--csv alice,bob --csv charlie" -> ["alice","bob","charlie"]. The current support for callables is insufficient for this.

This is done by introducing a 'customopt' class which can be extended for more powerful opts logic. All existing opt-types are converted to use this class, simplifying the fancyopts() logic.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dploch created this revision.Feb 7 2018, 8:38 PM

The fancyopts code is some of the oldest in Mercurial. We've been wanting to rewrite it for a while. This patch seems like an interesting and more powerful direction to take the parser.

Out of curiosity, do you have an intended use case in mind? Will that use case be in Mercurial itself or is this for an extension?

dploch added a comment.Feb 7 2018, 9:53 PM

The fancyopts code is some of the oldest in Mercurial. We've been wanting to rewrite it for a while. This patch seems like an interesting and more powerful direction to take the parser.
Out of curiosity, do you have an intended use case in mind? Will that use case be in Mercurial itself or is this for an extension?

I didn't have a use case for Mercurial itself in mind, but I wouldn't be surprised if there was one. My intended use case is the 'csv' flag example in the commit description: We have a lot of flags in our internal extensions that require the ["alice,bob", "charlie"] -> ["alice", "bob", "charlie"] behavior, so it would be really nice to be able to declare a shareable customopt for these instead of needing to individually wrap every applicable opts['flag'] lookup.

durin42 accepted this revision as: durin42.Feb 14 2018, 8:42 PM
durin42 added a subscriber: durin42.

I _really_ like where this is headed, but will refrain from queueing for now since it's a bit of a conflict of interest.

Friendly ping! This is my first commit so I'm not sure if more information or changes are expected; please let me know if there's anything I'm missing.

indygreg requested changes to this revision.Feb 21 2018, 12:42 AM

I like where this is going.

Out of curiosity, do you think it would be possible to implement an option that behaved like a boolean when given in isolation but also optionally accepted a value? My use case is I want hg serve --open to automatically open a web browser pointing at the started server and hg serve --open chrome to open Chrome instead of my default web browser. I'm not sure if that's a good idea to implement in the parser though. It could possibly lead to ambiguous argument parsing.

Anyway, I'd be happy to queue this. But I'd like to hear your reply about my review comments first in case you feel like improving this code as part of the refactor.

mercurial/fancyopts.py
225–226

Can we do isinstance(self.defaultvalue, (bool, types.NoneType)) instead?

232–233

callable is a built-in symbol in Python and should not be redefined.

259–262

isinstance() is preferred here. Although the integer check could be a bit wonky if Python 2's long ever comes into play.

313–317

Oh, your code was copied from here. I suppose it is mostly OK then. Although bringing this into modernity as part of the refactor wouldn't hurt...

This revision now requires changes to proceed.Feb 21 2018, 12:42 AM
dploch marked 4 inline comments as done.Feb 21 2018, 10:26 PM
dploch updated this revision to Diff 5981.

Out of curiosity, do you think it would be possible to implement an option that behaved like a boolean when given in isolation but also optionally accepted a value? My use case is I want hg serve --open to automatically open a web browser pointing at the started server and hg serve --open chrome to open Chrome instead of my default web browser. I'm not sure if that's a good idea to implement in the parser though. It could possibly lead to ambiguous argument parsing.

It feels like a bad idea. If we want the parsing to be unambiguous, then '--open' must always come at the end of the command line if the default option is desired. If there were a separate flag with the same semantics, you couldn't specify both: 'hg serve --open --foo' would pass "--foo" as the argument to --open.

If we stipulate that the optional argument only counts as an argument if it doesn't look like a flag (^-(-)?[^\d-].*$), it sort of works, but that feels pretty messy.

mercurial/fancyopts.py
259–262

It's actually wonkier than expected... :)

313–317

Sure, modernized.

In D2090#39120, @dploch wrote:

Out of curiosity, do you think it would be possible to implement an option that behaved like a boolean when given in isolation but also optionally accepted a value? My use case is I want hg serve --open to automatically open a web browser pointing at the started server and hg serve --open chrome to open Chrome instead of my default web browser. I'm not sure if that's a good idea to implement in the parser though. It could possibly lead to ambiguous argument parsing.

It feels like a bad idea. If we want the parsing to be unambiguous, then '--open' must always come at the end of the command line if the default option is desired. If there were a separate flag with the same semantics, you couldn't specify both: 'hg serve --open --foo' would pass "--foo" as the argument to --open.
If we stipulate that the optional argument only counts as an argument if it doesn't look like a flag (^-(-)?[^\d-].*$), it sort of works, but that feels pretty messy.

I agree, it's too hard to understand the parsing rules if we allow this. I actually explored this a bit with negating boolean flags and it's just Too Messy in my opinion.

indygreg accepted this revision.Feb 21 2018, 10:56 PM

Thanks for following up with the style changes!

mercurial/fancyopts.py
259–262

That's some PHP/JavaScript wonkiness :/

I liked the old form for its brevity and will change this as part of landing.

This revision is now accepted and ready to land.Feb 21 2018, 10:56 PM
dploch marked 2 inline comments as done.Feb 21 2018, 11:06 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Feb 22 2018, 8:22 AM
yuja added inline comments.
mercurial/fancyopts.py
258

Perhaps it's safer to make defaultvalue() a function returning
a copy of default, instead of passing a copy to _listopt().

If we make _listopt public and start putting it into the static command
table, things will go wrong.