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

Change E0259 to the new error format #35773

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Conversation

EugeneGonzalez
Copy link

Fixes #35514 as part of #35233.

Sorry about creating a new PR I was having a lot of troubles squashing the commit because I didn't properly branch the new feature.

r? @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (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.

@@ -10,5 +10,6 @@

extern crate collections;
extern crate libc as collections; //~ ERROR E0259
//~^ already imported
Copy link
Member

Choose a reason for hiding this comment

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

Is this working? I have a little doubt.

Copy link
Contributor

Choose a reason for hiding this comment

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

@EugeneGonzalez - please change this label to be include NOTE as shown in the blog post

@EugeneGonzalez
Copy link
Author

@jonathandturner and @GuillaumeGomez sorry for the delay, I just got back to school. The Note is now added to both lines that have notes.

@EugeneGonzalez
Copy link
Author

I noticed that #36100 implemented the bonus and that same approach could apply to the primary note of E0259. Should that also be added in?

@GuillaumeGomez
Copy link
Member

It depends. If you feel that you can do it, then just do it and we'll review once updated. We're not in a hurry.

Fixed E0259 unit test

Added name of conflict to E0259's note
@EugeneGonzalez
Copy link
Author

EugeneGonzalez commented Aug 31, 2016

Seems this failed because of #36138.

@sophiajt
Copy link
Contributor

@EugeneGonzalez - yeah, travis is having problems.

Btw, did you attempt the bonus by chance?

@EugeneGonzalez
Copy link
Author

@jonathandturner there wasn't a bonus associated with this one, but I adapted the solution used in #36100 to get the form of
'(conflicting crate name)' was already imported
on the primary label.

@sophiajt
Copy link
Contributor

sophiajt commented Aug 31, 2016

@EugeneGonzalez oh! okay. Just let me know what it's ready for review and I'll take a look (or @GuillaumeGomez will)

@EugeneGonzalez
Copy link
Author

EugeneGonzalez commented Aug 31, 2016

@jonathandturner it is ready to be looked at. One concern I have is the different wording used in the label. #36100 didn't use "was" while I did. It may be better to drop it for consistency, but I believe adding "was" makes a nicer label.

@sophiajt
Copy link
Contributor

@EugeneGonzalez - I like having the "was". It does read a little better. This looks good to go, so I'm going to go ahead and approve.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 31, 2016

📌 Commit d4ca561 has been approved by jonathandturner

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Aug 31, 2016
…urner

Change E0259 to the new error format

Fixes rust-lang#35514 as part of rust-lang#35233.

Sorry about creating a new PR I was having a lot of troubles squashing the commit because I didn't properly branch the new feature.

r? @GuillaumeGomez
bors added a commit that referenced this pull request Sep 1, 2016
@bors bors merged commit d4ca561 into rust-lang:master Sep 1, 2016
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.

5 participants