diff --git a/doc/source/devref/cli_option_guideline.rst b/doc/source/devref/cli_option_guideline.rst new file mode 100644 index 000000000..0d51ef68a --- /dev/null +++ b/doc/source/devref/cli_option_guideline.rst @@ -0,0 +1,246 @@ +==================== +CLI Option Guideline +==================== + +This document describes the conventions of neutron CLI options. + +General conventions +------------------- + +#. Option names should be delimited by a hyphen instead of a underscore. + This is the common guidelines across all OpenStack CLIs. + + * Good: ``--ip-version`` + * Not Good: ``--ip_version`` + +#. Use at least one required option for ``*-create`` command. If all options + are optional, we typically use ``name`` field as a required option. + +#. When you need to specify an ID of a resource, it is better to provide + another way to specify the resource like ``name`` or other reasonable field. + +#. If an attribute name in API is ``foo_id``, the corresponding option + should be ``--foo`` instead of ``--foo-id``. + + * It is because we usually support ID and ``name`` to specify a resource. + +#. Do not use ``nargs='?'`` without a special reason. + + * The behavior of ``nargs='?'`` of python argparse is a bit tricky + and it sometimes leads to unexpected option parsing which is + different from the help message. The detail is described + in :ref:`the Background section ` below. + +#. (option) Avoid using positional options as much as possible. + + * Positional arguments should be limited to attributes which will + be required in the long future. + +#. We honor existing options and should keep compatibilities when adding or + changing options. + +Options for boolean value +------------------------- + +Use the form of ``--option-name {True|False}``. + +* For a new option, it is recommended. +* It is suggested to use ``common.utils.add_boolean_argument`` in an + implementation. It allows ``true``/``false`` in addition to ``True``/``False``. +* For existing options, migration to the recommended form is not necessarily + required. All backward-compatibility should be kept without reasonable + reasons. + +Options for dict value +---------------------- + +Some API attributes take a dictionary. + +``--foo key1=val1,key2=val2`` is usually used. + +This means ``{"key1": "val1", "key2": "val2"}`` is passed in the API layer. + +Examples: + +* ``--host-route destination=CIDR,nexthop=IP_ADDR`` for a subnet +* ``--fixed-ip subnet_id=SUBNET,ip_address=IP_ADDR`` for a port. + +Options for list value +---------------------- + +Some attributes take a list. + +In this case, we usually use: + +* Define an option per element (Use a singular form as an option name) +* Allow to specify the option multiple times + +For Example, **port-create** has ``--security-group`` option. +``--security-group SG1 --security-group SG2`` generates +``{"security_groups: ["SG1", "SG2"]}`` in the API layer. + +This convention applies to a case of a list of dict. +``--allocation-pool`` and ``--host-route`` for a subnet are examples. + +Compatibility with extra arguments +---------------------------------- + +*extra arguments* supports various types of option specifications. +At least the following patterns needs to be considered when defining +a new option. For more detail, see :ref:`cli_extra_arguments`. + +* Normal options with value +* Boolean options : ``--foo True``, ``--bar=False`` +* List options : ``--bars list=true val1 val2``, ``--bars val1 val2`` +* Dict options : ``--foo type=dict key1=va1,key2=val2`` +* List of Dict options : ``--bars list=true type=dict key1=val1,key2=val2 key3=val3,key4=val4`` +* ``action=clear`` + +For normal options with value, there are four patterns to specify an option +as extra arguments. + +* ``--admin-state-up True`` (a space between option name and value) +* ``--admin-state-up=True`` (= between option name and value) +* ``--admin_state_up True`` (underscore is used as delimiter) +* ``--admin_state_up=True`` (underscore is used as delimiter) + +.. _background: + +Background +---------- + +There are a lot of opinions on which form of options are better or not. +This section tries to capture the reason of the current choice. + +Use at least one required option +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +As a convention, **neutron** CLI requires one required argument. + +If all options are optional in the API level and we have ``name`` field, +we usually use ``name`` as a required parameter. +Requiring at least one argument has the following benefits: + +* If we run ``neutron *-create`` without a required argument, we will have a + brief help message without detail option help. It is convenient. +* We can avoid miss operation by just hitting ``neutron *-create``. + Requiring at least one parameter is a good balance. + +Even though we can change this convention to allow to create a resource +without ``name`` field, it will bring confusions to existing users. + +There may be opinion that it is inconsistent with API level requirement +or Horizon behavior, but even if neutron CLI requires ``name`` field +there is no bad impact on regular users. Considering possible confusion +if we change it, it looks better to keep it as-is. + +Options for Boolean value +~~~~~~~~~~~~~~~~~~~~~~~~~ + +* ``--enable-foo``/``--disable-foo`` or similar patterns (including + ``--admin-state-down``) is not suggested because we need two exclusive + options for one attribute in REST API. It is meaningless. + +* It is not recommended to have an option only to specify non-default value. + For example, we have ``--shared`` or ``--admin-state-down`` options for + net-create. This form only works for ``*-create`` and does not work for + ``*-update``. It leads to having different options for ``*-create`` and + ``*-update``. + +* A flag option like ``--enable-dhcp`` (without value) also has a problem when + considering the compatibility with *extra argument*. We can specify + ``-enable-dhcp True/False`` or ``--enable-dhcp=True/False`` in the *extra + argument* mechanism. If we introduce ``--enable-dhcp`` (without value), + the form of ``-enable-dhcp True/False`` cannot be used now. + This is another reason we don't use a flag style option for a boolean parameter. + +.. _background-nargs: + +Avoid using positional options +------------------------------ + +The behavior of ``nargs='?'`` of python argparse is a bit tricky. +When we use ``nargs='?'``, if the order of command-line options is +swapped a bit, a command-line parser fails to parse the options easily. + +There are two examples of such failures. + +Example 1 shows that an actual behavior is different from +a way shown in a help message. The help message at ``[5]`` +shows we can use ``--bb CC``, but as you see at ``[7]`` +the arguent parsing fails. + +Example 1: + +.. code-block:: console + + In [1]: import argparse + In [2]: parser = argparse.ArgumentParser() + In [3]: parser.add_argument('--bb', nargs='?') + In [4]: parser.add_argument('cc') + + In [5]: parser.print_help() + usage: ipython [-h] [--bb [BB]] cc + + positional arguments: + cc + + optional arguments: + -h, --help show this help message and exit + --bb [BB] + + In [6]: parser.parse_args('--bb 1 X'.split()) + Out[6]: Namespace(bb='1', cc='X') + + In [7]: parser.parse_args('--bb X'.split()) + usage: ipython [-h] [--bb [BB]] cc + ipython: error: too few arguments + An exception has occurred, use %tb to see the full traceback. + + SystemExit: 2 + + +The second example shows how fragile ``nargs='?'`` is when a user +specifies options in a way different from the help message. +Most CLI usesr do not care the the order of command-line options, +so this fragile behavior should be avoided. + +Example 2: + +.. code-block:: console + + In [1]: import argparse + In [2]: parser = argparse.ArgumentParser() + In [3]: parser.add_argument('--a', help='option a') + In [4]: parser.add_argument('--b', help='option b') + In [5]: parser.add_argument('x', help='positional arg X') + In [6]: parser.add_argument('y', nargs='?', help='positional arg Y') + In [7]: parser.print_help() + usage: ipython [-h] [--a A] [--b B] x [y] + + positional arguments: + x positional arg X + y positional arg Y + + optional arguments: + -h, --help show this help message and exit + --a A option a + --b B option b + + In [8]: parser.parse_args('--a 1 --b 2 X Y'.split()) + Out[8]: Namespace(a='1', b='2', x='X', y='Y') + + In [9]: parser.parse_args('X Y --a 1 --b 2'.split()) + Out[9]: Namespace(a='1', b='2', x='X', y='Y') + + In [10]: parser.parse_args('X --a 1 --b 2 Y'.split()) + usage: ipython [-h] [--a A] [--b B] x [y] + ipython: error: unrecognized arguments: Y + An exception has occurred, use %tb to see the full traceback. + + SystemExit: 2 + + To exit: use 'exit', 'quit', or Ctrl-D. + To exit: use 'exit', 'quit', or Ctrl-D. + + diff --git a/doc/source/index.rst b/doc/source/index.rst index fa240033a..1d604310f 100644 --- a/doc/source/index.rst +++ b/doc/source/index.rst @@ -26,6 +26,7 @@ lower level programming details or APIs. :maxdepth: 2 devref/client_command_extensions + devref/cli_option_guideline History -------