pantsbuild / pants

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

python: a mandatory option is not really "advanced" (interpreter_constraints) #21397

Closed tdyas closed 2 weeks ago

tdyas commented 3 weeks ago

Pants currently requires users to set --python-interpreter-constraints and raises an exception if they do not. The option, however, is marked "advanced" which really should only apply to optional configuration with defaults most users will not touch, not to mandatory options with no default.

Remove the "advanced" marking so that this option is more discoverable by being output for ./pants help python.

thejcannon commented 2 weeks ago

Out of curiosity, could this be codified in a test?

tdyas commented 2 weeks ago

I suppose given we have examples of integration tests already which invoke Pants and check the resulting output.

benjyw commented 2 weeks ago

What would we test? And what would be the benefit vs the cost?

tdyas commented 2 weeks ago

What would we test? And what would be the benefit vs the cost?

I agree with Benjy. Nothing really to test here.

thejcannon commented 2 weeks ago

Sorry maybe I'm being naive, but isn't this that we think an option shouldn't be marked advanced AND not have a default? Seems like you could do that either in a test (by instrumenting the constructor) or just at type check time?

benjyw commented 2 weeks ago

All options have a registration-level default I think. We manually check that interpreter_constraints are explicitly provided, as a special case.

thejcannon commented 2 weeks ago

Ah I see! Ok ignore me 🙃