Enforce code style on CI with rustfmt

[biskit1]

Fixes #21

Note: I haven't done any testing as of yet- have to do some more reading about how it all works, and then will test it out later this week.

[0xazure]

What's the difference between --write-mode diff and --check? rustfmt's instructions for CI seem to prefer --check.

[biskit1]

yup my bad. I misread something.

[biskit1]

@0xazure - running cargo fmt changed some formatting in the main.rs file (not pushed), so presumably once merged, rustfmt via travis will make those changes?
Also did I insert the scripts and stuff in the right places?

[0xazure]

once merged, rustfmt via travis will make those changes?

No, Travis isn't set up to make and commit changes on its own, and generally we would want a person to do those tasks. It's more of a second verification step to ensure that the format is correct before PRs, etc get merged into master.

As part of this PR you'll also need to run rustfmt locally and commit any and all changes it makes, otherwise this PR won't pass Travis either 😛

[0xazure]

Travis config looks good, though I'd like to see the rustfmt steps applied to Linux-Beta and Windows-Beta as well.

Other than that, just run rustfmt locally, fix any problems the tool reports but can't fix by itself (there shouldn't be (m)any, I hope) and commit all the changes as part of this PR.

[biskit1]

@0xazure this should fix #21 (Enforce code style on CI with rustfmt), and #32 (Print tool versions on Travis). It should close my PR #43 (Print tool versions on Travis) without a merge, as I've added the changes from there to my original branch in this PR as per your suggestion in #43.

[0xazure]

Hey @biskit1 thanks for following up.

Unfortunately I think there was a miscommunication between #39 (this pull request) and #43.

To be a candidate to fix #21, #39 (this pull request) should:

• introduce the rustfmt component to our CI system
• contain commit(s) that fix any identified formatting issues so the checks pass for this pull request

Also, in light of the existence of #32, #39 could also:

• print the rustfmt version as part of CI

but this is not completely necessary for this pull request to be accepted.

You said:

I've added the changes from there to my original branch in this PR as per your suggestion in #43

but my comment in #43 was (in part):

I think it would be best if you included printing the version for rustfmt as part of #39

so I'm not sure what part of my suggestion you're referring to here. I did mention that branching pull requests from other active pull requests isn't generally something we want to do, simply because it can make conflict resolution more tedious, but I didn't mean to merge all the changes back into one branch and I'm sorry if I didn't communicate that clearly.

Pull requests are generally focused on adding/fixing/improving one thing; in the case of #39 (this pull request), it's implementing changes to fix #21 as I outlined above.

I would really like to see a fix for #32 land as a separate pull request, unrelated to this one. #43 is a good start towards this, but should be more focused on fixing #32 specifically (that is: focus on printing the clippy version). I think these two pull requests, #39 and #43 need to be disentangled a bit before either of them can be merged.

Let me know if you have any questions, and feel free to ping me over chat too!

[biskit1]

@0xazure Sorry for the confusion and mess 😝 I thought we didn't want to use 43 because of the fact that I had it branching off of this one. I think I also got confused by this paragraph from your review in 43:

And this is an issue with having many pull requests that deal with related pieces open at the same time 😛 . I think it would be best if you included printing the version for rustfmt as part of #39, and make reference to #32 as you do so; #32 is a general "CI tools should print their version" issue, and #39 introduces a new tool, so it should conform to #32 as well. These rustfmt additions are also preventing the Travis builds for this pull request from passing, so it would be better to keep them all in #39 along with the formatting fixes that will make the build pass.

I'm not sure why I branched 43 off of 39. I must have been thinking that it would make sense to add rustfmt in this PR, and then print the versions for both in a separate one, but that confuses the specific desired outcome per issue so I realize it doesn't really make sense and I shouldn't have done that to begin with. Considering that clippy was already integrated, I should have just added rustfmt in this PR, and printed clippy version in that one, and then maybe opened another issue to print rustfmt version.

So to clarify, what you're saying is
a) you'd like this PR (39) to fix 21 (enforce code style with rustfmt), but it's alright to have the rustfmt version printed out as part of this as well, and
b) you want a separate PR for 32 ('print clippy version') which will just print the clippy version (and have that change removed from this PR)

Considering 43 is currently branched off of this PR's branch, for the sake of simplicity can we just close 43 and I'll open a new PR to print the clippy version?

Sorry again for the confusion. Count on me to confuse the simplest things 🙄