Skip to content

Run all rust code through rustfmt#143

Closed
IanWhitney wants to merge 2 commits intoexercism:masterfrom
IanWhitney:rustfmt
Closed

Run all rust code through rustfmt#143
IanWhitney wants to merge 2 commits intoexercism:masterfrom
IanWhitney:rustfmt

Conversation

@IanWhitney
Copy link
Copy Markdown
Contributor

I've set up my Vim to run rustfmt whenever I save a file. It helps give
all of our code a consistent style which I think makes work easier.

But whenever I changed an old file, I would end up with a bunch of
commit noise. My commit might be changing a single test, but every line
of the file would get tweaked by rustfmt.

So I'm rustfmting everything to get us to a common baseline.

Still have to figure out how to make all future code use rustfmt.

I've set up my Vim to run rustfmt whenever I save a file. It helps give
all of our code a consistent style which I think makes work easier.

But whenever I changed an old file, I would end up with a bunch of
commit noise. My commit might be changing a single test, but every line
of the file would get tweaked by rustfmt.

So I'm rustfmting everything to get us to a common baseline.

Still have to figure out how to make all future code use rustfmt.
@IanWhitney
Copy link
Copy Markdown
Contributor Author

I don't see anything controversial, but I'm open to any feedback before merging.

Comment thread exercises/dominoes/tests/dominoes.rs Outdated
GotInvalid, // chain returned None
Correct,
ChainingFailure(Vec<Domino>), // failure to match the dots at the right side of one domino with
ChainingFailure(Vec<Domino>), /* failure to match the dots at the right side of one domino with */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this one's a little weird, since the comment spans two line and now one line has /* */ and the next has //. I wonder why rustfmt didn't apply the same change to line 14.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To make clear, I would not hold up the PR over this specific change, but am curious to know why it happened

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The answer is probably somewhere in here: https://github.com/rust-lang-nursery/rustfmt/blob/master/src/comment.rs

But I haven't spotted it.

I'm going to update this with a change that should be easier to read and won't be changed by rustfmt.

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Jun 14, 2016

nice, there are a few changes in here (nitpicky things like the presence or absence of spaces or trailing commas) that I've been wanting to see for a while now, good that they're getting done automatically!

I have a few questions about places where specific placements of newlines really help to visualize the input better. as I mentioned in line comments, I don't like having to fight an automated tool by reverting its change every time; is there a way we can signal it that the newlines should stay for those cases? you may have to bear with me on that question since I've not used rustfmt before :x

@IanWhitney
Copy link
Copy Markdown
Contributor Author

When I get a chance I want to see how/if rustfmt can be customized. Then maybe we can address some of the more problematic changes it made.

@IanWhitney
Copy link
Copy Markdown
Contributor Author

I decided that I didn't want to fight all the cases where our current formatting was appropriate. Closing.

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.

4 participants