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

rustup: fix nightly #433

Merged
merged 4 commits into from
Feb 24, 2016
Merged

rustup: fix nightly #433

merged 4 commits into from
Feb 24, 2016

Conversation

japaric
Copy link
Contributor

@japaric japaric commented Feb 21, 2016

See details in the commits.

Jorge Aparicio added 3 commits February 21, 2016 11:18
OsStr recently gained unstable inherent methods with the same name. This rename
makes sure we don't call the inherent methods but the methods provided by this
extension trait.
@yo-bot
Copy link

yo-bot commented Feb 21, 2016

Thanks for the pull request, and welcome! The team is excited to review your changes, and you should hear from @sru (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sru
Copy link
Contributor

sru commented Feb 21, 2016

Hello, and thanks for the pull request! The first commit (d752c17) is good, but other two "not good" (I can't find a word for it right now) because they can be done in several ways.

For missing debug implementations, I think it is best to implement those debug information (see #430).

For deriving Copy for ArgFlags, it does sound reasonable because ArgFlags is only a wrapper for underlying bit flags (which is cheap to copy, AFAIK). I don't know if the Copy trait was intentionally left out though (pinging @kbknapp). Also, adding Copy trait means that those calls to .clone() is unnecessary, as picked up by clippy in the Travis CI build.

The nightly build is still failing because of the unnecessary .clone() I mentioned above and because of useless format emitted by clippy.

@kbknapp
Copy link
Member

kbknapp commented Feb 22, 2016

Copy wasent left out on purpose, just overlooked. Adding copy is a great idea.

I'm also ok with the missing debug implementations, but if we end up needing to add the debug info for thone struts at some point we may add it. At some point in the future I'd like to look at only deriving debug implementations when the debug feature is used...buy that's just me thinking out loud, not set in stone.

The only thing I'm not sure about for this pr is the len->len_ changes. What's the gain?

Also like @sru said, thanks for putting this PR together!

@jedisct1
Copy link

Isn't the len() -> len_() change required since len() was added to OsString?

@japaric
Copy link
Contributor Author

japaric commented Feb 22, 2016

Isn't the len() -> len_() change required since len() was added to OsString?

Yes. OsString just gained unstable inherent len and is_empty methods. Calling str.len() will use the inherent method instead of the method in OsStrExt2 raising an error because the inherent method is unstable.

Renaming OsStrExt2 methods to something different than the inherent methods solves the problem.

@kbknapp
Copy link
Member

kbknapp commented Feb 22, 2016

Ah ok awsome I hadn't seen that! 👍

@japaric
Copy link
Contributor Author

japaric commented Feb 22, 2016

pushed a new commit fixing the clippy warnings

@sru
Copy link
Contributor

sru commented Feb 23, 2016

Hmm... I don't know why appveyor is failing now... @Vinatorul?

@kbknapp
Copy link
Member

kbknapp commented Feb 24, 2016

Not sure, I just restarted the build.

@kbknapp
Copy link
Member

kbknapp commented Feb 24, 2016

It seems like an issue with AppVeyor itself, and since this PR passed AppVeyor once already, I'm good merging without it this time.

@japaric Thanks for the contributions! Much appreciated!

@kbknapp
Copy link
Member

kbknapp commented Feb 24, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Feb 24, 2016

📌 Commit 56d14cb has been approved by kbknapp

homu added a commit that referenced this pull request Feb 24, 2016
rustup: fix nightly

See details in the commits.
@homu
Copy link
Contributor

homu commented Feb 24, 2016

⌛ Testing commit 56d14cb with merge a5ceee2...

homu added a commit that referenced this pull request Feb 24, 2016
rustup: fix nightly

See details in the commits.
@homu
Copy link
Contributor

homu commented Feb 24, 2016

☀️ Test successful - status

@homu homu merged commit 56d14cb into clap-rs:master Feb 24, 2016
@japaric japaric deleted the rustup branch February 24, 2016 13:30
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.

6 participants