Skip to content

Fixed #17823 #30937

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

Closed
wants to merge 3 commits into from
Closed

Fixed #17823 #30937

wants to merge 3 commits into from

Conversation

KalitaAlexey
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Contributor

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

@KalitaAlexey
Copy link
Contributor Author

I have a few questions. I fixed but and I formatted error_reporting.rs with rustfmt. Is it ok? I'd like to write test where error does not contain pattern. I see that currently compiletest has no such feature.

@alexcrichton
Copy link
Member

Could the rustfmt changes be separated out? Unfortunately they're large enough that github doesn't display the diff :(

@KalitaAlexey
Copy link
Contributor Author

@alexcrichton Yes. I can. Should I close this pull request?

@alexcrichton
Copy link
Member

Nah feel free to just force-push this branch to one of the changes and open another for the other change

@KalitaAlexey
Copy link
Contributor Author

@alexcrichton I don't understand you. I am new to git. May we chat somewhere?

@alexcrichton
Copy link
Member

Ah yeah what's here is fine, you may want to also try an interactive git rebase to remove the first two commits so the PR only contains the last one.

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned Aatch Jan 15, 2016
@@ -479,6 +479,14 @@ impl<'a, 'tcx> ErrorReporting<'tcx> for InferCtxt<'a, 'tcx> {
trace: TypeTrace<'tcx>,
terr: &TypeError<'tcx>)
-> DiagnosticBuilder<'tcx> {
fn is_simple_type<'tcx>(ty: Ty<'tcx>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a method on TyS rather than a function here - is_primitive maybe

@KalitaAlexey
Copy link
Contributor Author

@alexcrichton I already pushed commits. How to remove it?

@KalitaAlexey KalitaAlexey changed the title Fixed #17823. error_reporting is formatted with rustfmt Fixed #17823 Jan 16, 2016
@alexcrichton
Copy link
Member

The git rebase -i command should do the trick (followed by git push -f), and I found some good explanation of that online.

@KalitaAlexey
Copy link
Contributor Author

There is "all before you share your work with others." on that page. I already pushed changes to repository.

@alexcrichton
Copy link
Member

That's moreso a message for when you've pushed to the branch of a repo, these haven't been published yet as they're just a PR.

@KalitaAlexey
Copy link
Contributor Author

But my repository already has those commits. What should I do to erase them? Should I do another fork?

@KalitaAlexey
Copy link
Contributor Author

I know how to do that. I must close PR and open new with correct changes. Is it right?

@KalitaAlexey
Copy link
Contributor Author

#30953 New PR.

@nrc
Copy link
Member

nrc commented Jan 17, 2016

@KalitaAlexey you don't need to close the PR, a branch on your repo doesn't count as 'public', only a branch on a repo other people use. So you can force push to your old branch, keeping the PR open.

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