xtriggers: improve parser

[oliver-sanders]

Tolerate comas within strings and equals signs withing kwarg strings.

• Closes xtriggers and values with a comma breaks Cylc #5575
• Closes Debug print of xtriggers with a trailing comma in the flow.cylc definition is odd #5576

Check List

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests are included (or explain why tests are not needed).
• CHANGES.md entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch (we are skipping 8.1.5)

[ColemanTom]

I did a quick test of your function and it seems to work well for my tests. Obviously some unit tests to confirm a variety of cases have been tested would be good, but I'm sure you have that on the way. Thanks for the quick action on this one, it will help simplify some of the xtriggers we have.

[hjoliver]

Looks good, tested. Perhaps you could add a few more cases to the doctests (e.g. no trailing comma, whitespace).

[oliver-sanders]

Doctests for parse_xtrig_arglist already have examples with/without trailing comma. Whitespace doesn't influence the approach used.

Can't think of any other useful examples to test, covered all types, positional/keyword args, w/wo trailing comma.

[hjoliver]

Whitespace doesn't influence the approach used.

We could potentially break it by changing the approach in the future. However, no biggie in this case.

[oliver-sanders]

Added a (pointless 😁) whitespace test, a changelog and rebased.

[wxtim]

Should the following work?

>>> parse('alphabet, gimmel=["foo", 1, "qui"]')
 (['alphabet', '1', '"qui"]'], {'gimmel': '["foo"'})

The docs say

they can take arbitrary positional and keyword arguments

[oliver-sanders]

No, that shouldn't work because xtrigger signatures don't support all Python types (e.g. Lists):

cylc-flow/cylc/flow/parsec/validate.py

Lines 1108 to 1136 in e550ca4

	@classmethod
	def _coerce_type(cls, value):
	"""Convert value to int, float, or bool, if possible.
	Examples:
	>>> CylcConfigValidator._coerce_type('1')
	1
	>>> CylcConfigValidator._coerce_type('1.1')
	1.1
	>>> CylcConfigValidator._coerce_type('True')
	True
	>>> CylcConfigValidator._coerce_type('abc')
	'abc'
	"""
	try:
	val = int(value)
	except ValueError:
	try:
	val = float(value)
	except ValueError:
	if value == 'False':
	val = False
	elif value == 'True':
	val = True
	else:
	# Leave as string.
	val = cls.strip_and_unquote([], value)
	return val

More info: #5588

[ColemanTom]

No, that shouldn't work because xtrigger signatures don't support all Python types (e.g. Lists):

cylc-flow/cylc/flow/parsec/validate.py

Lines 1108 to 1136 in e550ca4

	@classmethod
	def _coerce_type(cls, value):
	"""Convert value to int, float, or bool, if possible.
	Examples:
	>>> CylcConfigValidator._coerce_type('1')
	1
	>>> CylcConfigValidator._coerce_type('1.1')
	1.1
	>>> CylcConfigValidator._coerce_type('True')
	True
	>>> CylcConfigValidator._coerce_type('abc')
	'abc'
	"""
	try:
	val = int(value)
	except ValueError:
	try:
	val = float(value)
	except ValueError:
	if value == 'False':
	val = False
	elif value == 'True':
	val = True
	else:
	# Leave as string.
	val = cls.strip_and_unquote([], value)
	return val

More info: #5588

I've not looked, but are these limitations covered in the documentation?

[hjoliver]

I've not looked, but are these limitations covered in the documentation?

Yes:

https://cylc.github.io/cylc-doc/stable/html/user-guide/writing-workflows/external-triggers.html#custom-trigger-functions

integer, float, boolean, and string arguments will be recognized and passed to the function as such

And you could probably use json.loads() internally to deserialize lists etc., if necessary.

[oliver-sanders]

And you could probably use json.loads() internally to deserialize lists etc., if necessary.

With limitations, including the one initially reported (trailing commas).

Since these signatures are intended to look and feel like the Python function signatures which they are ultimately configuring, I think the correct approach is to use Python to parse this signature:

#5588

This requires dropping support for unquoted "implicit" strings which I think we should probably deprecate as they are hard to support via any implementation.