This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2090

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

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

Download Raw Diff

Details

Reviewers

durin42
indygreg

Group Reviewers

hg-reviewers

Commits

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

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

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptFeb 7 2018, 8:38 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

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?

In D2090#34906, @indygreg wrote:

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.

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.

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.

In D2090#38775, @indygreg 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.

mercurial/fancyopts.py
259–262	It's actually wonkier than expected... :)
313–317	Sure, modernized.

In D2090#39120, @dploch wrote:

In D2090#38775, @indygreg 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.

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

Closed by commit rHG2ed36fec5321: fancyopts: add support for custom multi-arg opts in fancyopts.py (authored by dploch). · Explain Why

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.

yuja mentioned this in D2623: dispatch: adding config items for overriding flag defaults.Mar 4 2018, 9:21 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/fancyopts.py (93 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	5330		Feb 7 2018, 8:38 PM	★	★
Diff 2	5981		Feb 21 2018, 10:25 PM	★	★
Diff 3	5984	rHG2ed36fec53218a1aed29b6a01d259fb7b919bd5a	Feb 21 2018, 11:05 PM	★	★

Diff 5981

mercurial/fancyopts.py

	# fancyopts.py - better command line parsing			# fancyopts.py - better command line parsing
	#			#
	# Copyright 2005-2009 Matt Mackall <mpm@selenic.com> and others			# Copyright 2005-2009 Matt Mackall <mpm@selenic.com> and others
	#			#
	# This software may be used and distributed according to the terms of the			# This software may be used and distributed according to the terms of the
	# GNU General Public License version 2 or any later version.			# GNU General Public License version 2 or any later version.

	from __future__ import absolute_import			from __future__ import absolute_import

				import abc
	import functools			import functools
				import types

	from .i18n import _			from .i18n import _
	from . import (			from . import (
	error,			error,
	pycompat,			pycompat,
	)			)

	# Set of flags to not apply boolean negation logic on			# Set of flags to not apply boolean negation logic on
	else:			else:
	# -s/--str VALUE			# -s/--str VALUE
	parsedopts.append((flag, args[pos + 1]))			parsedopts.append((flag, args[pos + 1]))
	pos += 2			pos += 2

	parsedargs.extend(args[pos:])			parsedargs.extend(args[pos:])
	return parsedopts, parsedargs			return parsedopts, parsedargs

				class customopt(object):
				"""Manage defaults and mutations for any type of opt."""

				__metaclass__ = abc.ABCMeta

				def __init__(self, defaultvalue):
				self.defaultvalue = defaultvalue

				def _isboolopt(self):
				return False

				@abc.abstractmethod
				def newstate(self, oldstate, newparam, abort):
				"""Adds newparam to oldstate and returns the new state.

				On failure, abort can be called with a string error message."""

				class _simpleopt(customopt):
				def _isboolopt(self):
				return isinstance(self.defaultvalue, (bool, types.NoneType))

				indygregUnsubmitted Done Can we do `isinstance(self.defaultvalue, (bool, types.NoneType))` instead? indygreg: Can we do `isinstance(self.defaultvalue, (bool, types.NoneType))` instead?
				def newstate(self, oldstate, newparam, abort):
				return newparam

				class _callableopt(customopt):
				def __init__(self, callablefn):
				self.callablefn = callablefn
				super(_callableopt, self).__init__(None)
				indygregUnsubmitted Done `callable` is a built-in symbol in Python and should not be redefined. indygreg: `callable` is a built-in symbol in Python and should not be redefined.

				def newstate(self, oldstate, newparam, abort):
				return self.callablefn(newparam)

				class _listopt(customopt):
				def newstate(self, oldstate, newparam, abort):
				oldstate.append(newparam)
				return oldstate

				class _intopt(customopt):
				def newstate(self, oldstate, newparam, abort):
				try:
				return int(newparam)
				except ValueError:
				abort(_('expected int'))

				def _defaultopt(default):
				"""Returns a default opt implementation, given a default value."""

				if isinstance(default, customopt):
				return default
				elif callable(default):
				return _callableopt(default)
				elif isinstance(default, list):
				return _listopt(default[:])
				yujaUnsubmitted Not Done 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. yuja: Perhaps it's safer to make `defaultvalue()` a function returning a copy of default, instead of…
				# isinstance(True, int) returns True for some reason.
				elif isinstance(default, (int, long)) and not isinstance(default, bool):
				return _intopt(default)
				else:
				indygregUnsubmitted Done `isinstance()` is preferred here. Although the integer check could be a bit wonky if Python 2's `long` ever comes into play. indygreg: `isinstance()` is preferred here. Although the integer check could be a bit wonky if Python 2's…
				dplochAuthorUnsubmitted Done It's actually wonkier than expected... :) dploch: It's actually wonkier than expected... :)
				indygregUnsubmitted Not Done That's some PHP/JavaScript wonkiness :/ I liked the old form for its brevity and will change this as part of landing. indygreg: That's some PHP/JavaScript wonkiness :/ I liked the old form for its brevity and will change…
				return _simpleopt(default)

	def fancyopts(args, options, state, gnu=False, early=False, optaliases=None):			def fancyopts(args, options, state, gnu=False, early=False, optaliases=None):
	"""			"""
	read args, parse options, and store options in state			read args, parse options, and store options in state

	each option is a tuple of:			each option is a tuple of:

	short option or ''			short option or ''
	long option			long option
	default value			default value
	description			description
	option value label(optional)			option value label(optional)

	option types include:			option types include:

	boolean or none - option sets variable in state to true			boolean or none - option sets variable in state to true
	string - parameter string is stored in state			string - parameter string is stored in state
	list - parameter string is added to a list			list - parameter string is added to a list
	integer - parameter strings is stored as int			integer - parameter strings is stored as int
	function - call function with parameter			function - call function with parameter
				customopt - subclass of 'customopt'

	optaliases is a mapping from a canonical option name to a list of			optaliases is a mapping from a canonical option name to a list of
	additional long options. This exists for preserving backward compatibility			additional long options. This exists for preserving backward compatibility
	of early options. If we want to use it extensively, please consider moving			of early options. If we want to use it extensively, please consider moving
	the functionality to the options table (e.g separate long options by '\|'.)			the functionality to the options table (e.g separate long options by '\|'.)

	non-option args are returned			non-option args are returned
	"""			"""
	# convert opts to getopt format			# convert opts to getopt format
	onames = [name]			onames = [name]
	onames.extend(optaliases.get(name, []))			onames.extend(optaliases.get(name, []))
	name = name.replace('-', '_')			name = name.replace('-', '_')

	argmap['-' + short] = name			argmap['-' + short] = name
	for n in onames:			for n in onames:
	argmap['--' + n] = name			argmap['--' + n] = name
	defmap[name] = default			defmap[name] = _defaultopt(default)

	# copy defaults to state			# copy defaults to state
	if isinstance(default, list):			state[name] = defmap[name].defaultvalue
	state[name] = default[:]
	elif callable(default):
	state[name] = None
	else:
	state[name] = default

	# does it take a parameter?			# does it take a parameter?
	if not (default is None or default is True or default is False):			if not defmap[name]._isboolopt():
	if short:			if short:
	short += ':'			short += ':'
	onames = [n + '=' for n in onames]			onames = [n + '=' for n in onames]
	elif name not in nevernegate:			elif name not in nevernegate:
	for n in onames:			for n in onames:
	if n.startswith('no-'):			if n.startswith('no-'):
	insert = n[3:]			insert = n[3:]
	else:			else:
	for opt, val in opts:			for opt, val in opts:
	boolval = True			boolval = True
	negation = negations.get(opt, False)			negation = negations.get(opt, False)
	if negation:			if negation:
	opt = negation			opt = negation
	boolval = False			boolval = False
	name = argmap[opt]			name = argmap[opt]
	obj = defmap[name]			obj = defmap[name]
	t = type(obj)			if obj._isboolopt():
	if callable(obj):
	state[name] = defmap[name](val)
	elif t is type(1):
	try:
	state[name] = int(val)
	except ValueError:
	raise error.Abort(_('invalid value %r for option %s, '
	'expected int') % (val, opt))
	elif t is type(''):
	state[name] = val
	elif t is type([]):
	state[name].append(val)
	elif t is type(None) or t is type(False):
	indygregUnsubmitted Done 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... indygreg: Oh, your code was copied from here. I suppose it is mostly OK then. Although bringing this into…
	dplochAuthorUnsubmitted Done Sure, modernized. dploch: Sure, modernized.
	state[name] = boolval			state[name] = boolval
				else:
				def abort(s):
				raise error.Abort(
				_('invalid value %r for option %s, %s') % (val, opt, s))
				state[name] = defmap[name].newstate(state[name], val, abort)

	# return unparsed args			# return unparsed args
	return args			return args