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

Rustfmt librustc resolve #29387

Merged
merged 1 commit into from
Nov 15, 2015
Merged

Rustfmt librustc resolve #29387

merged 1 commit into from
Nov 15, 2015

Conversation

little-dude
Copy link
Contributor

Another rustfmt PR.
I ran rustfmt, then split the changes in multiple commits. First commit are the non-problematic changed. The others are all the little weirdness that caught my attention and could be discussed.

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

@little-dude
Copy link
Contributor Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned arielb1 Oct 26, 2015
@nrc
Copy link
Member

nrc commented Oct 26, 2015

re the 9th commit - when the function decl needs multiple lines we always split the args, I think this is fine - once a function decl gets this long the readability benefits of argument per line outweigh the benefits of keeping things on a few lines.

@nrc
Copy link
Member

nrc commented Nov 3, 2015

ping @little-dude Can you rebase this branch please? Did you need any more help with the comments?

@little-dude
Copy link
Contributor Author

@nrc sorry, it's just lack of time (getting married in a few days :D). I'll try to do this later today.

@little-dude
Copy link
Contributor Author

seems like github removed all you comments after I rebased... In one of them you were asking about the version of rustfmt I'm using:

commit 3ce2b840cf73253cedf86aaf3b6b7a71e27d6b79
Merge: 58ff0d8 be77f8a
Author: Marcus Klaas de Vries <mail@marcusklaas.nl>
Date:   Sat Oct 24 16:38:51 2015 +0200

    Merge pull request #522 from marcusklaas/writing-tests

    Add a brief overview of rustfmt tests

@nrc
Copy link
Member

nrc commented Nov 3, 2015

@little-dude no worries - not trying to rush you, just checking you didn't need anything from me. And congrats!

@nrc
Copy link
Member

nrc commented Nov 3, 2015

lib.rs:2484 - comments are misaligned

I think that is the only thing that needs fixing up now.

@little-dude
Copy link
Contributor Author

lib.rs:2484 - comments are misaligned

thanks, I'll fix this.

And congrats!

thank you!

@bors
Copy link
Contributor

bors commented Nov 4, 2015

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

@little-dude
Copy link
Contributor Author

@nrc I rebased against master and fixed the indentation of the comments that were misaligned. See last two commits.

If this is good for you I'll squash everything.

@nrc
Copy link
Member

nrc commented Nov 10, 2015

@little-dude looks goo, thanks! r+ with a squash

@bors
Copy link
Contributor

bors commented Nov 11, 2015

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

@little-dude
Copy link
Contributor Author

Arf. I'll rebase again.

@little-dude
Copy link
Contributor Author

@nrc: I rebased, could you review again please?

@nrc
Copy link
Member

nrc commented Nov 15, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 15, 2015

📌 Commit 889b0e9 has been approved by nrc

bors added a commit that referenced this pull request Nov 15, 2015
Another rustfmt PR.
I ran rustfmt, then split the changes in multiple commits. First commit are the non-problematic changed. The others are all the little weirdness that caught my attention and could be discussed.
@bors
Copy link
Contributor

bors commented Nov 15, 2015

⌛ Testing commit 889b0e9 with merge b7845f9...

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