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

Fix command. #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix command. #78

wants to merge 1 commit into from

Conversation

o0Ignition0o
Copy link
Owner

@o0Ignition0o o0Ignition0o commented Jan 13, 2020

This will allow us to to fix the formatting issues by running:

cargo-scout fix fmt

Although it won't apply to clippy(yet?), it will allow us to close #72

@o0Ignition0o o0Ignition0o force-pushed the fix branch 2 times, most recently from 18e05d3 to b45bf58 Compare January 13, 2020 15:01
@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Attention: Patch coverage is 93.24324% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.16%. Comparing base (196eb11) to head (9bc1e13).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
cargo-scout-lib/src/scout/mod.rs 86.20% 4 Missing ⚠️
cargo-scout-lib/src/utils.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   92.24%   92.16%   -0.08%     
==========================================
  Files           7        7              
  Lines         606      664      +58     
==========================================
+ Hits          559      612      +53     
- Misses         47       52       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@o0Ignition0o o0Ignition0o force-pushed the fix branch 7 times, most recently from fd57847 to 71977d6 Compare January 14, 2020 17:07
@o0Ignition0o
Copy link
Owner Author

A couple of tests in mod.rs and we're good to go !

@o0Ignition0o o0Ignition0o marked this pull request as ready for review January 16, 2020 20:01
@o0Ignition0o
Copy link
Owner Author

@ebroto @davidhewitt please let me know what you think :)
This pr allows us to run ‘cargo-scout fix fmt’.
I hope we can someday run ‘cargo-scout fix lint’ as well

@o0Ignition0o o0Ignition0o changed the title WIP: Tryouts on a fix command. Fix command. Jan 20, 2020
@davidhewitt
Copy link

Many thanks for this! I will give it a shot next time I need to format a PR and report back :)

Copy link
Contributor

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

Hey @o0Ignition0o, LGTM!

I've just left a comment about a doubt I have. Otherwise I tested it and it seems to be working fine :)

Great work!

Lint(LintOptions),
}

#[derive(StructOpt)]
enum FixCommand {
Fmt(LintOptions),
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to use FmtOptions here, right?

Copy link
Owner Author

@o0Ignition0o o0Ignition0o Jan 21, 2020

Choose a reason for hiding this comment

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

By LintOptions I mean something that returns lints, but I might have picked a poor name here.
The goal is to get anything that returns lints, and apply fixes on it.
Shoud I rename the options struct ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I could have explained myself better :)

I meant that right now cargo-scout fix fmt --help shows the same options as cargo-scout lint --help, which shows options that does not seem to belong to rustfmt, like

--no-default-features    Pass the no default features flag to clippy

On the other hand, FmtOptions seem to have the options related to rustfmt.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh I m sorry I havent seen your comments!

that makes sense indeed, I'm going to try to find a way to separate options from the logic, which will make it more reliable anyway

@cecton
Copy link

cecton commented Sep 17, 2020

It only worked for the imports for me

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.

Format changed code?
4 participants