Conversation
We chose But it would be very good to update it from |
|
thanks @seanprashad. Should we open an issue to update that? |
|
|
0xazure
left a comment
0xazure
left a comment
There was a problem hiding this comment.
Hey @biskit1!
this branches off of my iss-21 PR #39 branch. Not sure if this was the correct way to do it?
Generally this isn't correct; pull requests should branch off from and then be made against the main branch (in our case, master). The problem in not doing this is the pull request contains changes that aren't relevant to the topic of the pull request: in this case including the cargo fmt additions as well as printing version numbers.
I wanted to include the version check for the tool I added in that PR
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.
Wondering why not just
rustup component add clippy
The clippy component used to be known as clippy-preview, but now that clippy is considered 1.0 it is available on stable and it looks like its name has been changed. Apparently clippy-preview is now aliased to clippy, but we should definitely transition over to the new name. I can't actually find any information or documentation about this change other than rust-lang/rust-clippy#3502 changing the clippy README instructions, but it's definitely a recent change.
rustfmt should also be
rustfmt-preview?
The rustfmt docs say:
rustup component add rustfmt
so it looks like it's just called rustfmt, no -preview suffix.
|
Fixed with #39. |
|
Hey @biskit1, seem my comment on #39, it will not supersede this pull request. |
3 participants
Fixes issue #32 .
Note: this branches off of my iss-21 PR #39 branch. Not sure if this was the correct way to do it?
#39 has not been merged yet, and I wanted to include the version check for the tool I added in that PR, that's why I did it this way.
Also question: @seanprashad added
clippy-preview. Wondering why not justrustup component add clippy, and if the rustfmt should also berustfmt-preview?Couldn't find much about this online in the little digging I did...