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.

Sign up for GitHub

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

Jump to bottom

Disallow default_value and required for Option #88

Merged
merged 2 commits into from
Apr 6, 2018
Merged

Disallow default_value and required for Option #88

merged 2 commits into from
Apr 6, 2018

Conversation

Kerollmops
Copy link
Contributor

No description provided.

@Kerollmops
Copy link
Contributor Author

Tests passes on rustc 1.26-nightly on my macbook pro.

TeXitoi
TeXitoi approved these changes Apr 2, 2018
View reviewed changes
Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

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

Tests fails with cargo test --features nightly. I can't look at it now.

@TeXitoi
Copy link
Owner

TeXitoi commented Apr 2, 2018

Please add a line in the changelog

@Kerollmops
Copy link
Contributor Author

Kerollmops commented Apr 2, 2018 •
edited
Loading

We should probably bump the major number because this is not a backward compatible update, it will break current compiling code. https://semver.org/

@TeXitoi
Copy link
Owner

TeXitoi commented Apr 2, 2018

I've said in 0.2 that I can break compilation for better error handling, thus no major version change for that.

@TeXitoi
Copy link
Owner

TeXitoi commented Apr 2, 2018

The failing test also fail on master. Maybe syn and quote are not yet synchronised with nightly.

Only the first doc comment is given by syn.

rust-lang/rust#49545 may be related to this.

I didn't understand the problem I do t know where reporting the issue. Maybe syn? @dtolnay any idea?

@TeXitoi
Copy link
Owner

TeXitoi commented Apr 2, 2018

Required is also meaningless with option.

@Kerollmops Kerollmops changed the title Remove meaningless default value on option values Remove meaningless default value and required on Option Apr 2, 2018
TeXitoi
TeXitoi reviewed Apr 2, 2018
View reviewed changes
CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
# v0.2.7 (unreleased)

* Fail compilation when using `default_value` with `Option` ([#88](https://github.com/TeXitoi/structopt/pull/88)) by [@Kerollmops](https://github.com/Kerollmops)
Copy link
Owner

Choose a reason for hiding this comment

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

Not updated accordingly.

@Kerollmops Kerollmops changed the title Remove meaningless default value and required on Option Disallow default_value and required for Option Apr 2, 2018
@dtolnay dtolnay mentioned this pull request Apr 2, 2018
Closed
@dtolnay
Copy link

dtolnay commented Apr 2, 2018

Thanks for the ping. I filed rust-lang/rust#49596 to follow up.

@TeXitoi
Copy link
Owner

TeXitoi commented Apr 6, 2018

Fixed in nightly for our case.

Thanks for the contribution and your patience.

@TeXitoi TeXitoi merged commit 6a39e59 into TeXitoi:master Apr 6, 2018
Kerollmops
Kerollmops commented Apr 6, 2018
View reviewed changes
CHANGELOG.md
@@ -1,3 +1,7 @@
# v0.2.7 (unreleased)

* Fail compilation when using `default_value` and `required` with `Option` ([#88](https://github.com/TeXitoi/structopt/pull/88)) by [@Kerollmops](https://github.com/Kerollmops)
Copy link
Contributor Author

@Kerollmops Kerollmops Apr 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

s/and/or 😞

Copy link
Owner

Choose a reason for hiding this comment

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

amended

TeXitoi pushed a commit that referenced this pull request Apr 6, 2018
@Kerollmops @TeXitoi
Disallow default_value and required for Option (#88)
d59117a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TeXitoi TeXitoi TeXitoi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kerollmops @TeXitoi @dtolnay