[GitHub] [buildstream] cs-shadow commented on pull request #1438: Tristan/remote cli options

[GitBox <gi...@apache.org>]

cs-shadow commented on pull request #1438:
URL: https://github.com/apache/buildstream/pull/1438#issuecomment-775560360


   Hi @gtristan, I haven't had a chance to look at the rest but here are my thoughts from the UI/UX perspective:
   
   tl;dr I think it's preferrable to use singular option names in both cases - i.e. whether multiple remotes are allowed or just one.
   
   ---
   
   When only one value is expected, we always use the singular option name. So, that's sorted. For the remainder of this comment, I'll assume that we're dealing with an option that needs to accept multiple values.
   
   Click offers two ways of handling multiple values for options:
   
   1. [multiple=True](https://click.palletsprojects.com/en/7.x/options/?highlight=multiple#multiple-options), which looks like `--option value1 --option value2 ...`
   
   2. [nargs](https://click.palletsprojects.com/en/7.x/options/?highlight=multiple#multi-value-options), which looks like `--option value1 value2 ...`
   
   In my opinion singular option names (like  `--remote`) make more sense for (1) and plural option names (like `--remotes`) for (2). This is because `--remotes R1 R2 R3` sounds natural while `--remotes R1 --remotes R2` sounds rather awkward. Similarly, `--remote R1 --remote R2` sounds better than `--remote R1 R2 R3`
   
   Currently, we are using `nargs` exclusively for postional arguments and `multiple` excusively for options. As such, my suggestion would be to use singular names whenever dealing with options, regardless of how many values they accepts.
   
   In this pull request, seems like you're already doing so, like `--artifact-remote` and `--source-remote`.
   
   Lastly, `--ignore-project-artifact-remotes` also seems good to me. This is because it is a flag and cannot be repeated. And the plural name correctly indicates that it will operate on all remotes and not a specific one.
   
   ---
   
   PS: Using `multiple=True` for options is somewhat forced on us because options are not allowed to have nargs < 0.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org