Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove advanced flags from python-repos subsystems #16318

Closed
wants to merge 2 commits into from

Conversation

asherf
Copy link
Member

@asherf asherf commented Jul 27, 2022

Currently, if you try to get help for this subsystem, ./pants help python-repos you will get nothing useful and the user needs to repeat the command with the help-advanced directive (happened to me right now).
My thinking is: It is somewhat pointless to have a subsystem where all the options are advanced, it makes sense if there are multiple options and some are advanced and some or not.
But I don't think we should allow options scopes that only have advanced options since it a little confusing to users trying to get help about it.

This is just an opinion, feel free to close this PR.

Currently, if you try to get help for this subsystem, `./pants help python-repos` you will get nothing useful and the user needs to repeat the command with the help-advanced directive (happened to me right now).
My thinking is: It is somewhat pointless to have a subsystem where all the options are advanced, it makes sense if there are multiple options and some are advanced and some or not.
But I don't think we should allow options scopes that only have advanced options since it a little confusing to users trying to get help about it.

This is just an opinion, feel free to close this PR.
@jsirois
Copy link
Contributor

jsirois commented Jul 27, 2022

@asherf you're getting burnt by the macos-10.15 brownout. Fix is in #16317.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now, to avoid the weirdness @asherf mentions.

A more general solution might be to show advanced help automatically if there are no non-advanced options.

I've been wondering what the benefit of the "advanced" concept is. Initially it was for "options more likely to be specified on the cli" vs "options more likely to be specified in config". Now with bash autocompletion we actually have a use for this distinction!

@Eric-Arellano
Copy link
Contributor

I've been wondering what the benefit of the "advanced" concept is. Initially it was for "options more likely to be specified on the cli" vs "options more likely to be specified in config". Now with bash autocompletion we actually have a use for this distinction!

Hm, so then should we instead fix this by your suggestion right above?

A more general solution might be to show advanced help automatically if there are no non-advanced options.

@benjyw
Copy link
Contributor

benjyw commented Jul 29, 2022

Hm, so then should we instead fix this by your suggestion right above?

Maybe? It's true that these are properly "advanced" options in the now-useful sense. I.e., we wouldn't want to bash-complete them, they would just be noise. Or maybe we do and my distinction is not significant after all? I feel like we could go around on that for a while...

But anyway, yeah, may be better to show advanced options proactively in such cases.

@Eric-Arellano
Copy link
Contributor

We have A LOT of options. Now that we have autocomplete, I bias towards thinking the distinction is worth it. But I could be convinced.

@asherf
Copy link
Member Author

asherf commented Jul 29, 2022

I added #16352
but I still think this PR is viable since I think those options are not "advanced", at least not to me....
just my 2c.
but feel free to close this PR if it makes sense to keep these options as advanced.

Eric-Arellano pushed a commit that referenced this pull request Aug 1, 2022
also added some missing type annotations because why not.

see further discussions: #16318 (review)
![Screen Shot 2022-07-29 at 9 53 44 AM](https://user-images.githubusercontent.com/1268088/181775284-7ceba133-0e9c-4171-aef6-02852ce12a9d.png)
@Eric-Arellano
Copy link
Contributor

but I still think this PR is viable since I think those options are not "advanced", at least not to me....
just my 2c.

Given our definition of "is this likely to be set by everyday users, or only admins?", I think these would be advanced. Ideally everyday users don't have to think about indexes! Wdyt?

@asherf
Copy link
Member Author

asherf commented Aug 1, 2022

but I still think this PR is viable since I think those options are not "advanced", at least not to me....
just my 2c.

Given our definition of "is this likely to be set by everyday users, or only admins?", I think these would be advanced. Ideally everyday users don't have to think about indexes! Wdyt?

that definition makes total sense to me.

@asherf asherf closed this Aug 1, 2022
@asherf asherf deleted the cosmo branch August 1, 2022 16:01
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants