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

docs: Clarify corner caseses with default values #2896

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Conversation

epage
Copy link
Member

@epage epage commented Oct 16, 2021

This is meant to lower the chance of confusion with cases like #2714 and #1586

This is not meant to be exhaustive, looked at the mentioned cases in
that issue and pattern matched on other ones mentioning "is present".

When working on this, I wanted to make sure we limit our **NOTE:**s to not lower their value, so I moved the YAML description down into the examples which seems more fitting and some cases already do this.

@pksunkara
Copy link
Member

NOTE: This argument is considered present when there is a [Arg::default_value]

I disagree with this. The initial implementation was very buggy because of incomplete design. We had fixed many of those bugs for v3 but it still behaves wrongly because of the original design.

What we need is a tiny bit refactor on the design for how this works to solve the linked issue. This is actually one of the reason why I have all those default value related issues marked as 3.0 (Let's keep continuing to discuss the deferral part in other places). They will all be fixed by this design refactor.

@epage
Copy link
Member Author

epage commented Oct 16, 2021

NOTE: This argument is considered present when there is a [Arg::default_value]

I disagree with this. The initial implementation was very buggy because of incomplete design. We had fixed many of those bugs for v3 but it still behaves wrongly because of the original design.

What we need is a tiny bit refactor on the design for how this works to solve the linked issue.

That is best discussed in the Issue.

@pksunkara
Copy link
Member

pksunkara commented Oct 18, 2021

I looked at #2897 a bit more and I think I can get it done by the end of upcoming weekend. Let's wait on merging this until then.

@epage
Copy link
Member Author

epage commented Oct 18, 2021

I looked at #2897 a bit more and I think I can get it done by the end of upcoming weekend. Let's wait on merging this until then.

I'd rather we not keep a branch long-lived for the sake of a possibility. If you end up fixing this, it can be reverted then.

@pksunkara
Copy link
Member

Give me a few days as I asked. It's not long. I lose track of things that need reverts when I fix them.

epage added 2 commits October 23, 2021 16:04
This is meant to lower the chance of confusion with cases like clap-rs#2714 and clap-rs#1586.

This is not meant to be exhaustive, looked at the mentioned cases in
that issue and pattern matched on other ones mentioning "is present".
@pksunkara
Copy link
Member

Let's remove the second commit and merge this. #2714 and #1586 and #1906 are purely bugs in the conflict handling code. I have them fixed without even needing breaking changes.

@epage
Copy link
Member Author

epage commented Oct 25, 2021

Let's remove the second commit and merge this. #2714 and #1586 and #1906 are purely bugs in the conflict handling code. I have them fixed without even needing breaking changes.

We can revert that commit when a fix becomes available.

bors r+

@pksunkara
Copy link
Member

We don't add documentation when it is a big fix right?

@epage
Copy link
Member Author

epage commented Oct 25, 2021

We don't add documentation when it is a big fix right?

We are documenting a pitfall within the current behavior. If/when that pitfall is removed, we can update it.

@bors
Copy link
Contributor

bors bot commented Oct 25, 2021

Build succeeded:

@bors bors bot merged commit 29972e1 into clap-rs:master Oct 25, 2021
@epage epage deleted the docs branch October 25, 2021 17:59
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.

2 participants