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

set applicability #53655

Merged
merged 1 commit into from
Aug 31, 2018
Merged

set applicability #53655

merged 1 commit into from
Aug 31, 2018

Conversation

jcpst
Copy link
Contributor

@jcpst jcpst commented Aug 24, 2018

Update a few more calls as described in #50723

r? @estebank

@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 @estebank (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2018
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I saw a couple of places where we need to fix the wording of the suggestion.

The changes seem correct. Can you go through all of the changes and make sure they look kind of like this?

err.span_suggestion_with_applicability(
    span,
    msg,
    code,
    applicability,
);

Other than that r=me.

sugg_msg,
new_snippet);
new_snippet,
Applicability::MachineApplicable);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix the formatting as you did below? (each arg on it's own line, 4 spaces indented, trailing comma on every line, closing ); on it's own line)

span,
"you can try using the variant's enum",
enum_path,
Applicability::MachineApplicable);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: move the closing ); to it's own line and leave a trailing comma here

@jcpst
Copy link
Contributor Author

jcpst commented Aug 24, 2018

Thanks for reviewing @estebank ! This is my first PR against rust-lang/rust. I can likely update more of these calls soon. Let me know if you need anything else!

@estebank
Copy link
Contributor

estebank commented Aug 24, 2018

@jcpst thank you for the change! This is very useful. The only thing that I would bother you with is if you could squash all your changes onto one commit (this will require you to do a force push on your repo, double check that everything went as you expected before doing so).

@bors
Copy link
Contributor

bors commented Aug 24, 2018

☔ The latest upstream changes (presumably #53662) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank
Copy link
Contributor

There was also a merge conflict now, so you will have to rebase against latest master.

@rust-highfive

This comment has been minimized.

@jcpst
Copy link
Contributor Author

jcpst commented Aug 25, 2018

Thanks for the help, although I didn't mean to add those submodule changes. Unfortunately, while researching and trying to figure out how to remove them from my PR, I seem to have my local copy in an undesirable state. Not sure how I remove them

The best idea I have right now seems to be starting over with these changes.

@estebank
Copy link
Contributor

You can nuke your local branch and create a fresh one with the same name, apply your changes and force push. That will overwrite what appears in the PR. Just don't lose your changes 😊

@jcpst jcpst force-pushed the with_applicability branch 2 times, most recently from 137da37 to 8c6d151 Compare August 26, 2018 04:04
@rust-highfive

This comment has been minimized.

@jcpst
Copy link
Contributor Author

jcpst commented Aug 26, 2018

Ok, I think I have the process down now. Let me know if this needs any other changes!

@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 26, 2018

📌 Commit 1f421d6 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Aug 28, 2018
set applicability

Update a few more calls as described in rust-lang#50723

r? @estebank
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 28, 2018
set applicability

Update a few more calls as described in rust-lang#50723

r? @estebank
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 29, 2018
set applicability

Update a few more calls as described in rust-lang#50723

r? @estebank
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 29, 2018
set applicability

Update a few more calls as described in rust-lang#50723

r? @estebank
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 30, 2018
set applicability

Update a few more calls as described in rust-lang#50723

r? @estebank
bors added a commit that referenced this pull request Aug 31, 2018
Rollup of 20 pull requests

Successful merges:

 - #51760 (Add another PartialEq example)
 - #53113 (Add example for Cow)
 - #53129 (remove `let x = baz` which was obscuring the real error)
 - #53389 (document effect of join on memory ordering)
 - #53472 (Use FxHash{Map,Set} instead of the default Hash{Map,Set} everywhere in rustc.)
 - #53476 (Add partialeq implementation for TryFromIntError type)
 - #53513 (Force-inline `shallow_resolve` at its hottest call site.)
 - #53655 (set applicability)
 - #53702 (Fix stabilisation version for macro_vis_matcher.)
 - #53727 (Do not suggest dereferencing in macro)
 - #53732 (save-analysis: Differentiate foreign functions and statics.)
 - #53740 (add llvm-readobj to llvm-tools-preview)
 - #53743 (fix a typo: taget_env -> target_env)
 - #53747 (Rustdoc fixes)
 - #53753 (expand keep-stage --help text)
 - #53756 (Fix typo in comment)
 - #53768 (move file-extension based .gitignore down to src/)
 - #53785 (Fix a comment in src/libcore/slice/mod.rs)
 - #53786 (Replace usages of 'bad_style' with 'nonstandard_style'.)
 - #53806 (Fix UI issues on Implementations on Foreign types)

Failed merges:

r? @ghost
@bors bors merged commit 1f421d6 into rust-lang:master Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants