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

implements issue #2356 #3252

Merged
merged 1 commit into from
Aug 25, 2012
Merged

Conversation

Vincent-Belliard
Copy link
Contributor

This commit modify the error message when an identifier is not found but it can be found in the class.
The old message was:
issue_2356.rs:22:16: 22:21 error: unresolved name: xyzt
The new message is:
issue_2356.rs:22:16: 22:20 error: unresolved name: xyzt.Did you mean: self.xyzt

I started looking at rust sources yesterday. It's my first commit which modify the sources. I hope I followed all the coding rules.

@catamorphism
Copy link
Contributor

@Vincent-Belliard -- Thanks for the patch! The first thing I'm noticing is that you're using while loops instead of for loops. Generally, in Rust, we use for loops because they're less error-prone (you can't forget to increment the index), so long as you're incrementing the index in a uniform way. Can you rewrite with for and rebase against HEAD of incoming, then submit another patch? Thanks again!

@ghost ghost assigned catamorphism Aug 23, 2012
@catamorphism
Copy link
Contributor

By the way, there's a style guide at https://github.com/mozilla/rust/wiki/Note-style-guide but it's still pretty sketchy at the moment.

@Vincent-Belliard
Copy link
Contributor Author

I use for loops now. I just left the first while because we need to
inspect ribs in reverse order.

I read the few style guides I found but there are not too many things
inside.

On 08/23/2012 09:35 PM, Tim Chevalier wrote:

By the way, there's a style guide at
https://github.com/mozilla/rust/wiki/Note-style-guide but it's still
pretty sketchy at the moment.


Reply to this email directly or view it on GitHub
#3252 (comment).

@catamorphism
Copy link
Contributor

@Vincent-Belliard : Looks great for the most part! I made a few comments as line-by-line notes on your two commits. Since there are a few issues to fix, I figured I would let you fix them and submit a new commit -- if it was just a matter of rebasing off HEAD, I would do it myself, but it's a bit more involved. Hopefully my comments are understandable -- if not, just ask (or find me on IRC -- my nick there is tjc).

Sorry this has turned out to be a bit involved, but if you fix the (minor) issues and resubmit a commit -- preferable combining the two commits into a single one -- I'll merge it. Thanks!

@Vincent-Belliard
Copy link
Contributor Author

I modified the commit to include all you remarks. I rebased it against head of incoming.

Thank you for for taking some of your time to help me beginning with Rust.

catamorphism added a commit that referenced this pull request Aug 25, 2012
@catamorphism catamorphism merged commit 390a31a into rust-lang:incoming Aug 25, 2012
@catamorphism
Copy link
Contributor

Merged, thank you!

@catamorphism
Copy link
Contributor

Sorry, forgot one more thing: I'll add it myself in this case, but in general, every commit should come with a regression test that ensures that the change that was made behaves correctly. Occasionally this would be difficult, but usually it's not hard.

bors pushed a commit to rust-lang-ci/rust that referenced this pull request May 15, 2021
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jan 7, 2024
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.

None yet

2 participants