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

Run 'cargo fmt' to format code #489

Merged
merged 1 commit into from Mar 20, 2019

Conversation

Projects
None yet
3 participants
@Atul9
Copy link
Contributor

Atul9 commented Mar 19, 2019

No description provided.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #489 into master will decrease coverage by 0.62%.
The diff coverage is 24.39%.

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
- Coverage    27.3%   26.67%   -0.63%     
==========================================
  Files          64       64              
  Lines        7582     8713    +1131     
==========================================
+ Hits         2070     2324     +254     
- Misses       5512     6389     +877
@BaptisteGelez
Copy link
Member

BaptisteGelez left a comment

Hey, thank you! Would you want to try to add it to the CI too? (it's fine if you don't, we can totally do that in another PR)

@Atul9 Atul9 force-pushed the Atul9:format-using-cargo-fmt branch from 1e19883 to b9d3903 Mar 19, 2019

@Atul9

This comment has been minimized.

Copy link
Contributor Author

Atul9 commented Mar 19, 2019

Hi @BaptisteGelez I would like to add it. I am new to Plume project. Can you please point me to where in the .travis.yml file can cargo fmt be added?

@Atul9 Atul9 force-pushed the Atul9:format-using-cargo-fmt branch from b9d3903 to acc1419 Mar 19, 2019

@BaptisteGelez

This comment has been minimized.

Copy link
Member

BaptisteGelez commented Mar 19, 2019

You can probably add a new item at the end of the jobs section, in the "test and coverage" stage (I'm not sure if this indication is clear enough, tell me if it is not 😅)

@igalic

This comment has been minimized.

Copy link
Member

igalic commented Mar 19, 2019

given that the usual way to run rustfmt is in "check" mode, we can probably run it as first job. and from what i gather, rustfmt doesn't care about features, and just formats all code? or? did it format the test code, too?

@Atul9

This comment has been minimized.

Copy link
Contributor Author

Atul9 commented Mar 20, 2019

If the code isn't formatted ('rustfmt' will exit with 1 if there is an error in formatting - https://github.com/rust-lang/rustfmt#running) then is it still ok to go to the build stage?
Suggestion: We can add a check stage which will run before the build stage.
@BaptisteGelez @igalic What do you think?

@Atul9 Atul9 force-pushed the Atul9:format-using-cargo-fmt branch from 24f7509 to 6c4790d Mar 20, 2019

@Atul9

This comment has been minimized.

Copy link
Contributor Author

Atul9 commented Mar 20, 2019

@BaptisteGelez @igalic I tried adding cargo fmt but the CI fails for weird reason.

I tried locally but I didn't get this error - https://travis-ci.org/Plume-org/Plume/jobs/508992511#L584 & nor did it fail for the build before adding cargo fmt to travis.yml.

I tried to fix that but the build fails - https://travis-ci.org/Plume-org/Plume/jobs/509005663#L589

The build for this PR had passed before adding cargo fmt to travis build stages - https://travis-ci.org/Plume-org/Plume/builds/508519598

I suggest we can add cargo fmt to CI in another PR.

@BaptisteGelez

This comment has been minimized.

Copy link
Member

BaptisteGelez commented Mar 20, 2019

OK, no problem. I'll merge this as soon as you'll have reverted the changes to .travis.yml. And thank you again for your work. 🙂

@Atul9 Atul9 force-pushed the Atul9:format-using-cargo-fmt branch from a73c2cf to acc1419 Mar 20, 2019

@Atul9

This comment has been minimized.

Copy link
Contributor Author

Atul9 commented Mar 20, 2019

@BaptisteGelez done. I have updated the PR.

@BaptisteGelez BaptisteGelez merged commit b945d1f into Plume-org:master Mar 20, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.