Copilot commented on code in PR #13570:
URL: https://github.com/apache/trafficserver/pull/13570#discussion_r3871948113
##########
tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py:
##########
@@ -0,0 +1,119 @@
+'''
+Verify traffic_ctl command line parsing for the reload options that take a
+variable number of values, --directive (-D) and --data (-d).
+
+Options declared with MORE_THAN_ZERO_ARG_N used to consume every remaining
+token, so any option written after -D was silently swallowed as a directive
+value and never parsed. -D therefore had to be the last option, and -D could
+not be combined with -d. These runs assert on the JSONRPC request that
+traffic_ctl builds (printed by -f rpc), because the subject under test is the
+command line parsing rather than the server side handling of the reload.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
This new file places a module docstring before the ASF license header. In
ASF projects the license header is typically required to be the first content
in the file; consider moving the license comment block above the docstring (or
converting the docstring into a comment below the license) to avoid
tooling/compliance issues.
##########
doc/developer-guide/internal-libraries/ArgParser.en.rst:
##########
@@ -104,6 +104,33 @@ To add options to the parser or current command:
This function call returns the new :class:`Option` instance. (0 is also number
of arguments expected)
+.. Note::
+
+ For options, the number of arguments may also be one of the following,
which mirror the
+ ``nargs`` values of Python's ``argparse``:
+
+ ================================
=======================================================
+ Value Meaning
+ ================================
=======================================================
+ ``AT_MOST_ONE_ARG_N`` Zero or one value (``argparse``
``nargs='?'``)
+ ``MORE_THAN_ZERO_ARG_N`` Zero or more values (``argparse``
``nargs='*'``)
+ ``MORE_THAN_ONE_ARG_N`` One or more values (``argparse``
``nargs='+'``)
+ ================================
=======================================================
+
+ An option taking a variable number of values stops collecting when it
reaches a token
+ naming another option of the same command, so options written afterwards
keep their own
+ arguments. Use ``AT_MOST_ONE_ARG_N`` rather than ``MORE_THAN_ZERO_ARG_N``
for an option
+ whose value is optional, otherwise it also consumes the positional
arguments of its
+ command.
+
+ A token naming another option is not a value for a fixed number of
arguments either. An
+ option written where a value is expected leaves the value missing, which is
reported as a
+ usage error rather than the option being consumed and applied as the value.
+
+ A ``--`` token stops option recognition for the values being collected,
which is how a
+ value beginning with ``-`` is passed. Note this differs from the POSIX
``--``: it does
+ not end the value list nor force the remainder to be positional arguments.
Review Comment:
The docs explain that ``--`` stops option recognition 'for the values being
collected', but they don’t spell out an important implication for
variable-arity options: once ``--`` is seen while collecting a variable-length
value list, the collector will not stop at later option tokens (because option
recognition stays disabled), so it will consume to the end of the command line.
Consider adding a sentence making that behavior explicit to avoid surprising
API consumers.
##########
doc/appendices/command-line/traffic_ctl.en.rst:
##########
@@ -456,11 +456,17 @@ Display the current value of a configuration record.
.. note::
- ``-D`` uses variable-argument parsing and must appear as the **last
option**
- on the command line. Any flags placed after ``-D`` will be consumed
as directive
- values. ``-D`` and ``-d`` cannot be combined in the same invocation
due to this
- same constraint. Use ``-d`` with full YAML when you need both
directives and
- inline content in a single reload request.
+ ``-D`` accepts values until the next option or the end of the command
line, so it
+ may appear anywhere among the options and can be combined with ``-d``
— directives
+ and inline content merge under the same config key:
+
+ .. code-block:: bash
+
+ $ traffic_ctl config reload -D myconfig.id=foo --monitor
+ $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig:
{rules: [a]}'
+
+ To pass a directive value that begins with ``-``, place ``--`` before
it; every
+ token after ``--`` is taken as a value rather than an option.
Review Comment:
The sentence 'every token after ``--``' can be read as globally ending
option parsing for the whole command. In this parser, ``--`` affects value
collection for the current option, and for variable-arity options (like ``-D``)
it effectively causes the option to consume the remainder of the command line,
preventing any subsequent options from being recognized. Consider clarifying
this explicitly and (optionally) recommending the `--directive=-value` form
when callers need to keep specifying options after a dash-prefixed
directive-like value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
