run rustfmt on the repository #398(Updated)

[prertik]

Repository was formatted using rustfmt-nightly. The max_width needed to be increased to remove errors of width being more than the default 120. Some error still persist like:
error: left behind trailing whitespace
---> /mdBook/src/utils/mod.rs:178

and with the fix of code snippets being destroyed from doc comments.

[prertik]

@budziq @azerupi how about this one?

[azerupi]

The tests passed, I will take a look at the output to see if everything is good 👍

[azerupi]

@prertik I think we should remove the rustfmt.toml file altogether. It was meant for an old version of rustfmt, but it has changed significantly since then and most of the options are probably irrelevant now.

Could I ask you to remove the config file and run rustfmt again? :)

[prertik]

Yeah about rustfmt.toml, I had to configure it to pass the test as you can see certain changes. If we remove rustfmt.toml, the rustfmt would operate in default condition which is having max_width:120 which would fail to format mdBook/src/preprocess/links.rs as it has codes with max_width more than 120 and secondly, the problem in previous pull request would again arise i.e. rustfmt by default would again format code snippets in the doc comments. I opened an issue on rustfmt about this, a collaborator told me make changes in the rustfmt.toml file.

[Michael-F-Bryan]

Wouldn't it make more sense to just shorten the overly long lines instead of increasing the max width above 120 characters? If you have a really long string literal, you can break it over a newline by adding a \ to the very end.

Like this:

let some_really_long_string: &'static str = "this is a very long and uninteresting string \ 
  which I've wrapped over multiple lines.";

Also, in doc comments you typically want to wrap them at 80 characters anyway. I know there are one or two examples with really long overhanging lines, but it'd probably be better to either move the comments above or delete them outright if they don't add anything extra to the example... I think uncle bob is quoted as saying, "comments are apologies for badly written code [or poor naming]".

[azerupi]

Yes, I think it would be better to drop the config file. Resolve the issues with lines being too long and see if the output is acceptable. If the default style works for us, we should embrace it.

[prertik]

Thanks @Michael-F-Bryan for the feedback. Alright @azerupi, I will do that. I will send a new pull request and will close this one.

[budziq]

@prertik For future reference you can use git rebase and git push --force to completely rewrite the PR without need to close and open another one. Actually, I would suggest to reopen this PR and try this way yourself. If you'll have any problems with git I'll help you 👍 .

[prertik]

Thanks @budziq. Actually, i used to do git rebase before. But, in one repo, I ran into some problems due to some merge conflicts which i couldn't solve. So, i ignored it. But, i will definitely try it again. Surely, will ask you for any git help. 👍

[budziq]

The only way to stop rustfmt doing this

And how about setting fn_call_style to visual?
@prertik

[prertik]

Yeah, I guess it takes care of the extra line from block(default). But, I think the block has been made default for a reason i.e. being community standard.

[prertik]

Shouldn't we follow community standard? In Ruby, community standard is held at highest order.

[budziq]

But, I think the block has been made default for a reason i.e. being community standard.

Be that as it may, but rustftmt.toml facility was also made for a reason ;).

Please note that the rust community standard is in constant flux (4months ago its output on our repo was different and arguably/subjectively nicer).

Ruby, community standard is held at highest order.

It had a little more time to coalesce. Ours is stil wip.

Imho additional 2lines per invocation is just adding noise.
Just my 0.02$ :)

[prertik]

So, what should I do @budziq? Should I add rustfmt.toml and redo everything? :(

[budziq]

Should I add rustfmt.toml and redo everything?

If there would be some changes it would not be redoing everything a single run of

find ./src  -iname "*.rs"  | xargs rustfmt

should do the trick (if you're on a nix'y environment)

But anyhow I would wait on a decision from owner 👍

[azerupi]

and redo everything

Redoing everything should be relatively easy, no? rustfmt comes with a cargo sub-command to run it on the whole crate: cargo fmt.

I agree with everyone that some lines that are expanded would be better left as one line.If we can play with the configuration file and find some options that work better, great! Otherwise, I would be inclined to merge with the options that result in the best formatting and open an issue on rustfmt to give feedback.

[prertik]

So, just to be clear, I have to change the fn_call_style to visual by adding rustfmt.toml and then run rustfmt. Is that all?

[Michael-F-Bryan]

See what everything looks like when you tweak fn_call_style and run cargo fmt to reformat the entire project again. Then look at what changed with git diff HEAD and see if the changes seem alright to you.

Personally I don't mind what style we use, the biggest thing for me, and most probably the others, is that hitting save in my editor (I have format-on-save turned on) or running cargo fmt to format the entire project won't pollute PRs with unnecessary code churn.

[prertik]

How about I change every indentation to visual rather than block. Looks like everyone prefers visual?
Like here Configurations.md as you can see in chain_indent and others.

[prertik]

This looks fine to me. How about you @budziq, @azerupi, @Michael-F-Bryan?

[Michael-F-Bryan]

looks good to me 👍

[prertik]

This line format_strings = true is for future. When new lines of code are pushed containing long strings. The strings would be formatted automatically using format_strings otherwise manual formatting needs to be done for the long strings because of max_width error.

[budziq]

👍

[azerupi]

This looks a little strange.

[prertik]

Oh yeah, about this I made a array_layout = "Visual" in the rustfmt.toml because everyone here seemed to prefer the visual layout rather than the block. I think it looks OK. If you follow the codes from lines 67 to down, it looks structured and having uniform "Visual" layout, I think would be better in this repo as I think it creates a linear structure of codes.

That's my 2 cents on this.

[azerupi]

I think it's pretty good. There are still a couple of weird edge cases, but we can refine the config later if we find better defaults.

[Michael-F-Bryan]

I think it's pretty good. There are still a couple of weird edge cases, but we can refine the config later if we find better defaults.

@azerupi, does that mean this is good to merge?

[azerupi]

Yes, I think we can merge this PR. We can still tweak the settings when we discover better defaults.

I wonder if we can find a way to force or encourage the use of rustfmt for PRs. Maybe a pass in Travis? The only problem is that I don't want to block PRs on style, but I do want to make it obvious for us and the author of the PR that rustfmt was not run.

[Michael-F-Bryan]

Hmm... that's hard. Usually I have my editor set up so every time I hit save it'll format the document. Then every now and again you run cargo fmt and cargo clippy to clean things up.

I guess we could periodically make a cargo fmt PR, but that's not exactly ideal...

[prertik]

Yeah, I totally agree with @Michael-F-Bryan. Periodically running cargo fmt and using the configuration file present as rustfmt.toml in this repo would be ideal.

[Michael-F-Bryan]

Yes, I think we can merge this PR. We can still tweak the settings when we discover better defaults.

@azerupi, awesome! I'll let you or @budziq hit the big green "merge" button then because I don't have collaborator/owner status.

[budziq]

done. Thanks @prertik !