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

Candidates for renaming before v3 #2164

Closed
1 of 12 tasks
pksunkara opened this issue Oct 10, 2020 · 8 comments
Closed
1 of 12 tasks

Candidates for renaming before v3 #2164

pksunkara opened this issue Oct 10, 2020 · 8 comments
Labels
C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@pksunkara
Copy link
Member

pksunkara commented Oct 10, 2020

  • AppSettings
    • ArgsNegateSubcommands -> ?
    • GlobalVersion -> PropagateVersion
    • HelpRequired -> RequireHelp
    • SubcommandRequired -> RequireSubcommand
    • ArgRequiredElseHelp -> DisplayHelpWhenNoArg
    • SubcommandRequiredElseHelp -> DisplayHelpWhenNoSubcommand
    • AllowLeadingHyphen -> AllowHyphenValues (did this in Arg already)
    • SubcommandPrecedenceOverArg -> GiveSubcommandPrecedenceOverArg
    • SubcommandsNegateReqs -> ?
    • UnifiedHelpMessage -> UnifyHelpMessage
    • ColoredHelp -> ColorHelp / UseColoredHelp / ?
    • TrailingVarArg -> ?
  • Make App::generate_usage (a new function) consistent with App::render_* (also new)
@pksunkara pksunkara added C-enhancement Category: Raise on the bar on expectations C: settings labels Oct 10, 2020
@pksunkara pksunkara added this to the 3.0 milestone Oct 10, 2020
@pksunkara pksunkara pinned this issue May 26, 2021
@pksunkara pksunkara changed the title Possible candidates for renaming before v3 Candidates for renaming before v3 Jul 14, 2021
@epage
Copy link
Member

epage commented Jul 22, 2021

RE GlobalVersion

A benefit to this name is clap already has the term "global" that I think translates to this.

RE RequireHelp and RequireSubcommand

The thing I dislike here is that one "requires" is for developers and the other is for users. It'd be nice if we had a way of distinguishing these.

Maybe RequireHelp should be ExpectHelp like Option::expect?

This should also probably be renamed to ExpectAbout because the help argument was renamed to about.

RE ArgRequiredElseHelp

I like the consistency in using Require. Maybe RequireArgElseHelp

@epage
Copy link
Member

epage commented Jul 22, 2021

So my current table

  • ArgsNegateSubcommands
  • GlobalVersion
  • HelpRequired -> ExpectAbout
  • SubcommandRequired -> RequireSubcommand
  • SubcommandRequiredElseHelp -> RequireSubcommandElseHelp
  • ArgRequiredElseHelp -> RequireArgElseHelp
  • AllowLeadingHyphen: rename the Arg version back. This is more accurate than "Hyphen values" in my mind
  • SubcommandPrecedenceOverArg
  • SubcmmandsNegateReqs -> SubcommandNegateRequires
  • ColoredHelp -> ColorHelp
  • AllArgsOverrideSelf -> ArgsOverrideSelf (all is redundant, this is set on the app)
  • UnifiedHelpMessage -> UnifyHelpMessage

@kbknapp
Copy link
Member

kbknapp commented Oct 2, 2021

My general thoughts are, "What is least surprising or what is most likely to be searched for in the docs when wanting capability X." With a strong preference for consistency (back to my least surprising point) over perfect naming. So if we are using a verb/noun in some context, once the user has internalized that meaning with regard for clap, seeing that same verb or noun should have the same result.

I tend to think in terms of Noun_Verb(Past Participle)[_Context If Needed] when it's an attribute applied to an item, or [Verb_Noun] when it's instruction for clap to perform an action. There are several instances where this ins't the case, and those are ones I'd lean to towards changing, if any.

It could be argued that we should have different structure based on if the action is at parser build time, parse time, or validation time...as well as if it's a setting for clap, the developer, or the end user. But naming is one of the most bike-shedable things in CS so I'm partial to just leave well enough alone.

Likewise I tend to think that unless a name is actively causing a problem, or conflicting with some other naming scheme we've been using it's a low priority for change. Churn is more detrimental to downstream users than a less than perfect name.

Just my 2 cents.

@epage
Copy link
Member

epage commented Oct 2, 2021

My general thoughts are, "What is least surprising or what is most likely to be searched for in the docs when wanting capability X." With a strong preference for consistency (back to my least surprising point) over perfect naming. So if we are using a verb/noun in some context, once the user has internalized that meaning with regard for clap, seeing that same verb or noun should have the same result.

Another consideration for having a good scheme and having the feature stand out by being first (usually a noun) is it makes it easier to scan and discover new features when dealing with a specific domain (e.g. its easy to find Settings related to subcommands when they start with Subcommand)

Likewise I tend to think that unless a name is actively causing a problem, or conflicting with some other naming scheme we've been using it's a low priority for change. Churn is more detrimental to downstream users than a less than perfect name.

I've been impacted by both sides of this. Churn is rough. Similarly, inconsistent and undiscoverable names are also rough. I recently ran into this with nom where I didn't know features existed and did things in very round about ways because the naming was bad, inconsistent, and the docs didn't help towards discoverablity (I've been creating issues and prepping PRs to help improve this)


For v3, probably my biggest care about is if we started some renames and, if so, is it in a polished enough state or should we change it one way or the other. Anything else I can see pushing off to a later release. I guess I'd also throw in "having an API style guide" so new features are more likely to be right from the beginning.

@epage
Copy link
Member

epage commented Oct 4, 2021

One rename we should do for v3 is make App::generate_usage (a new function) consistent with App::render_* (also new)

epage added a commit to epage/clap that referenced this issue Oct 15, 2021
This is inline with all of our other help-related functions that return
strings.

This is a part of clap-rs#2164

BREAKING CHANEG: `App::generate_usage` (added in v3) ->
`App::render_usage`.
@epage
Copy link
Member

epage commented Oct 16, 2021

Based on kbknapp's post, I recommend we wrap this up with the following renames:

  • HelpRequired -> AboutExpected: using the .expect verbiage to distance this from the word "require" which has a strong runtime connotation to users and we've rena med Arg::help to Arg::about (which personally I dislike because I prefer the naming consistency than perfect accuracy for finding this in docs)
  • AllowLeadingHyphen: rename the Arg version back to this. This is more accurate than "Hyphen values" in my mind
  • SubcmmandsNegateReqs -> SubcommandNegateRequires
  • AllArgsOverrideSelf -> ArgsOverrideSelf (all is redundant, this is set on the app)
  • mut_arg -> arg_mut (_mut should be a suffix), new this release

If I were to prioritize this, I would say

  • AllowLeadingHyphen: rename the Arg version back to this. This is more accurate than "Hyphen values" in my mind and is the less disruptive route for clap2 users
  • mut_arg -> arg_mut (_mut should be a suffix, we should clarify this is a get and not set), new this release

If we keep with NounVerb, there are other renames we can do to be more consistent with that but people have already been living with it, they can live with it a little longer.

I did not see any other renames that I would undo because of the background / preference towards Setting::NounVerb

@epage
Copy link
Member

epage commented Nov 3, 2021

Another thought on AllowLeadingHyphen vs AllowHyphenValues is #1210 which, in the discussion, includes supporting alternative argument sigils, like Windows style /. I've not dug into the code to confirm this but I wonder if we can take inspiration from SubcommandPrecedenceOverArg on the naming, like ValuePrecedenceOverArg

@pksunkara
Copy link
Member Author

Proposing to punt this to v4 to get 3.0.0-rc.0 out this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

3 participants