pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.26k stars 626 forks source link

Deprecate implicit append feature of list options #9509

Open Eric-Arellano opened 4 years ago

Eric-Arellano commented 4 years ago

For a list option, if you add a value as a string without [], we will add the value:

[python-setup]
interpreter_compatibility = "CPython>=3.7"

This will add CPython>=3.7 to the default of CPython>=3.6, which is surprising and confusing because it blurs the distinction between str options vs. list options.

At the same, we have a more explicit way of doing the same thing:

[python-setup]
interpreter_compatibility.add = ["CPython>=3.7"]

--

The config file case is clear that it's better to use the explicit .add style. Where the current affordance is a little more valuable is on the command line to avoid dealing with shell quoting:

./pants --python-setup-interpreter-compatibility='CPython>=3.7'

This is easier to write than:

./pants --python-setup-interpreter-compatibility='+["CPython>=3.7"]'

Still, the command line suffers from the same problems of being surprising to the users and muddling the distinction between str and lists, so I argue we should deprecate this (mis)feature across the board.

jsirois commented 4 years ago

The other case to consider is PANTS_ env var configuration. In that case, interpreting a trailing _ADD, etc. would be straightforward;e.g.: PANTS_CONFIG_FILES_ADD=pants.remote.toml ./pants .... It would seem that doing the same for the CLI flags would be tricky requiring something like adding synthetic -add, etc suffix options that do not display in help. The consistency might be nice though instead of the current support for +[] and -[] which differs in syntax from the toml.

Eric-Arellano commented 4 years ago

Overall, +1 to replacing +[] and -[] with -add and -remove for the CLI and env vars.

interpreting a trailing _ADD, etc. would be straightforward;e.g.: PANTS_CONFIG_FILES_ADD=pants.remote.toml

This makes sense to allow leaving off the [] in that case. I think we'd want to allow for either of these:

$ PANTS_CONFIG_FILES_ADD=pants.remote.toml
$ PANTS_CONFIG_FILES_ADD='["pants.remote.toml"]'

Ditto on allowing this for the CLI flag.

--

Would you want to support this affordance in config files?

[python-setup]
interpreter_compatibility.add = "CPython>=3.7"

I don't love that, but I think the consistency with the CLI and env var are worth it, and this is at least unambiguous.

jsirois commented 4 years ago

I do think consistency would be great. I like learning less when I can and I think new users do too.

Things could get a bit nutty to support type purityish. You could imagine sticking with python words and supporting append, appendLeft, extend, extendLeft, etc. That's certainly wordy though.

Eric-Arellano commented 4 years ago

You could imagine sticking with python words and supporting append, appendLeft, extend, extendLeft, etc. That's certainly wordy though.

Oof, yes, I would much rather stick with only add and remove, even if that means the value is str | List[str].

I like learning less when I can and I think new users do too.

Agreed.

I view this as an argument that we should always expect List[str] instead of str when using .add and .remove because we don't have to teach users of this special case, but I'm fine with adding the sugar because there's at least no ambiguity and shell quote escaping is so clunky.

We can call it out as a special case like Benjy does in the blue info box here: https://pants.readme.io/docs/options#section-list-values. People can ignore this sugar if they want.