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

feat: add default_value_os_t #3333

Merged
merged 1 commit into from Jan 24, 2022
Merged

feat: add default_value_os_t #3333

merged 1 commit into from Jan 24, 2022

Conversation

omjadas
Copy link
Contributor

@omjadas omjadas commented Jan 24, 2022

Add default_value_os_t attribute

The order of suffixes allows us to preserve the original builder function name.

This is a part of #2813

@epage
Copy link
Member

epage commented Jan 24, 2022

The only thing I'm unsure of is which suffix (_t or _os) should be last.

Mind sharing your reasoning for which you chose?

@omjadas
Copy link
Contributor Author

omjadas commented Jan 24, 2022

The only thing I'm unsure of is which suffix (_t or _os) should be last.

Mind sharing your reasoning for which you chose?

I chose default_value_t_os in order to be consistent with default_value_if_os and default_value_ifs_os, where _os comes last.

@epage
Copy link
Member

epage commented Jan 24, 2022

Thanks for the explanation!

I don't think precedence carries much weight though because this is a new suffix that is similar to _os.

In giving time to mull over this, I think we should swap this and always have _t at the end. My reason is so we preserve the original builder method name for easier searching, rather than splitting the method name in two. This means derive-specific suffixes should always come last.

@omjadas omjadas changed the title feat: add default_value_t_os feat: add default_value_os_t Jan 24, 2022
@epage epage merged commit 86c83d2 into clap-rs:master Jan 24, 2022
@epage
Copy link
Member

epage commented Jan 24, 2022

v3.0.12 is released with this change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants