As a followup to #39 and as explained here It would be nice to add a test with a diff containing several changes to a .rs file and one or more changes to an other file extension.

the --no-default-features flag should be caught and passed to clippy

It might be useful to have cargo output lints in json, just a clippy does, in order to allow CI or other programs to run cargo-scout and use its output.
It should also be put behind a feature flag.

As an overall refactoring effort, using nom might ease git diff parsing (and help us towards #41)

Nom is really cool to use since 5.0, maybe we could try to use it!

Currently, cargo-scout is able to run on the staged modifed and added files, but only on the unstaged modified files (since the unstaged added files are created as untracked).

This behaviour is a bit incoherent with clippy's usage, where even untracked files are checked.

Ideally, we'd need clippy to run on untracked files.
The problem is that git diff cannot check untracked files as is.

some cargo feature flags don't work in workspaces.

Once #12 is complete, we can let cargo-scout know if it should run clippy on subfolders so the flags work correctly, or stay at the root level.

For now we wait until clippy is done running before we try to display anything on the command line, which is pretty inconvenient (and we have no idea if cargo-scout is running at all).

Is there a way to stream stdout and stderr while the command is running ?

Maybe we could try to pipe the output to the cli ?

The options keep adding up, and one parser has a constructor, the other one doesnt. let's use a builder pattern instead !

As pretty well explained here

There are a couple of spots where we use AsRef<std::path::Path>.
It would probably be more readable if we added a use statement on top, and replace occurences with AsRef<Path>

As mentioned by @ebroto in this comment, it would probably make much more sense to diff against HEAD instead of master by default.

It would require us to change the default value of the branch option from master to HEAD.

We would probably need to update the README.md file, and perhaps some documentation and tests as well.

StructOpt Provides a simple way to handle command line parameters, and put them in a struct.

It would be really cool it we could use it instead of the current implementation, which does not use custom_derive.

If anyone has any question and/or wants to take a stab at this, please comment the issue, I would be happy to provide mentoring and help :)

As mentionned in this comment cargo scout Error is expected to implement std::error::Error.

This is something that would probably only require us to add an impl Display to it :)

The --no-default-features flag cannot be used in clippy with a workspace directory.
cargo-scout should therefore walk through each workspace's members.

In order to avoid increasing run time, we might find a way to not run clippy on workspace members that aren't targeted by the git diff

As explained by @ebroto here, we could leverage the 1.40 non_exhaustive feature in some of our enums and types.

We should bump our Minimum Supported Rust Version as well if we wanna do so.

Having a lib.rs and a main.rs would be a nice first steps towards integration tests, as explained in #54

Splitting this would also allow us to reason about what we want to expose, and add documentation to exposed structures and traits. And would be a nice step towards having a Contributing.md file or a wiki.

the git sections function is becoming more and more messy. There is probably a way to refactor it and have a clean output.

Tests will allow us to refactor it, let's see if we can split it and do a better pattern matching.

There currently isn't any test for the error.rs file as we can see here

Once #58 is complete, cargo-scout errors will implement the error trait.

This issue will become a nice first contribution, and a good step to learn how to write tests in rust

If anyone wants to pick this one up, please let me know !
I'll happily answer any questions, and / or have a chat with you :D

How to proceed ?

As you can see in our already existing tests

there are 3 Phases:

• Setup: This is the phase where we prepare everything before running our tests. It involves creating our structures, preparing them to work as expected, and set up our expected result (this is the most important step)
• Run: This is the phase where we actually call the function we're trying to test.
• Assert: This is the phase where we make sure our actual result equals the expected one.

How to add tests to error.rs ?

Let's pretend we want to cover Error generation from String.
Here's a list of the three phases we want to add in this context:

• Setup: We first want to create a String.
• Run: We now create an error from the string
• Assert: We can make sure the error description matches the expected one.

Here's what the test for a Command error would look like :

#[cfg(test)]
mod scout_tests {
    use super::*;
    #[test]
    fn test_command_error() {
        // Setup
        let test_string_error = "this is a test error".to_string();
        let expected_error = Error::Command(test_string_error.clone());

        // Run
        // into() will call From<String> implicitely
        let actual_error: Error = test_string_error.into();

        // Assert
        assert_eq!(expected_error.description(), actual_error.description());
    }
}

If you want to wee me write more tests, have a look at this video :)

As can be seen in the fix documentation and in the clippy github page there’s a way for us to not only display the lints, but also fix them!

A first use case could be behind an option flag, like cargo-scout -f or cargo-scout —fix

A really awesome one would be by using the official fix command, like cargo fix —scout !

It’s a pretty big one, which will probably require a couple of steps ^^

Now that we have some cool code coverage, we can see where we have a nice code coverage, and where there are some holes.

There is room for improvement in clippy.rs. We could add a test in preview mode which would be a nice first step !

https://codecov.io/gh/o0Ignition0o/cargo-scout/src/master/cargo-scout-lib/src/linter/clippy.rs

For now the git parser only finds edit files, and doesn't handle new files.
This means we miss a lot of possibly lints.
We should be able to parse created files and handle it's lints.

Do you think it would be possible to add a flag/mode to format the changed code? This would be really helpful alongside clippy warnings to keep PRs high quality.

(A lot of projects would pick up a lot of changes that would be irrelevant to the PR if I run cargo fmt, so formatting just my diff would be great.)

If clippy has to be run with a flag that is not compatible with workspaces (such as --no-default-features for example), cargo-scout will have to walk through all of the directories to run clippy.
It therefore requires a list of members.

The neqo cargo.toml file can serve as an example for the parser.

Needs to be investigated, it's probably clippy related

As mentionned here

Let's try to play around with the cargo crate or the clippy crate and see what can be done.

There's a lot of cover_skip in the codebase because commands cannot be easily tested:

    // Skipped from code coverage
    // because an external command
    // cannot be easily unit tested
    #[cfg_attr(tarpaulin, skip)]

We might want to try working on our own command abstraction, that would return a Result<OutputStream, ErrorStream> so we can provide stubs for the tests.

It would allow for more coverage, and we could thus identify and reproduce cornercases easily.

So far we've been mostly using println!() to write things.

As the project size is growing, maybe we can now consider using a log crate.
There's a lot of loggers out there, we want to find one that is simple to use and futureproof (pun intended)

Right now we add any file that has a diff to the list.
We should only add rust files to the list because it will later allow us to run clippy only when needed (and on needed workspace members).

Maybe we could add a continue statement if the file_name doesn't end with .rs ?
https://github.com/o0Ignition0o/cargo-scout/blob/master/src/git.rs#L91

It would be really awesome if we could add a test to it :)

The project is getting bigger and it would be a good idea to split separate parts (tests in this case) into a separate location.

As explained in #36 and in this neqo comment, an unstable feature available on nightly only actually allows us to pass --no-default-features as expected, and to get relevant lints even if a build has already been done.

We might want to add a --preview flag that would allow us to run a nightly version and make the tool much more useful while the features we need are being stabilized.

The correct command we can already run on nightly cargo +nightly clippy-preview -Z unstable-options --no-default-features -- -W clippy::pedantic behaves exactly the way we wish ! 🎉

As explained here let's bikeshed ScoutOptions and figure out a proper way to link Linter, Config and Options in a Cargo Scout so we have the best traits and impls !

The cargo parser is currently using ./Cargo.toml as shown here.

It would be nice if we could pass a specific flag like -t and/or --cargo-toml which would allow us to pass a path to the Cargo.toml file, for example:
cargo-scout --cargo-toml "./foo/bar/Cargo.toml"

The steps to reproduce are pretty close from what has been done when adding the --no-default-features flag in #24.

A youtube video explaining how to do so (and actually passing the --no-default-features flag) is available here : https://www.youtube.com/watch?v=vrCuCnHYep0

Feel free to let me know if you have any question :)

Parsing the Cargo.toml and figuring out if it is being run in a workspace will later allow us to use correct flags when running clippy

in order to be able to use cargo-scout programmatically, it might be nice to have it warn when lints are found, and not error out.

It would be put behind a flag

When only 1 line is being edited, the parser reports at least 4 or 5 lines changed eg:

--- a/src/clippy.rs
+++ b/src/clippy.rs
@@ -124,3 +124,5 @@ mod tests {
         assert_eq!(expected_lints, lints(clippy_output));
     }
 }
+
+// this is a test

becomes

            Section {
                file_name: "src/clippy.rs".to_string(),
                line_start: 124,
                line_end: 129,
            },

Maybe there's a better way to handle the diff parsing and get the exact lines changed.

The problem with Into is that it's not sized, so making this change would probably require me to dig into Associated types( We would need to create an Impl Linter<P> where P: Into<PathBuf> + Sized, so I'm a bit scared to do it right now :/

OTOH it would allow us to allow more inputs (such as &str for example) so it would be really cool

Originally posted by @o0Ignition0o in #74

We're slowly adding more and more options to cargo-scout, and it might be a bit hard to remember them all.

clippy is doing something interesting in their arguments parsing by splitting arguments until they hit -- and change the arguments purpose.

Maybe we can try to bikeshed something like that:

parse the presence (or not) of +nightly (cargo option).
parse until the first -- and make it VCS arguments (git options).
parse until the second -- and make it Project arguments (cargo toml options)
parse until the end and make it Linter arguments (clippy options)

We could even take out the last given argument as a working directory/current_dir path.

Would that be a better overall and more generic approach to linters and options ?

cargo-scout should be allowed to be passed --all-features and pass it to clippy

A live stream on how to fix #11, which almost requires the same steps as fixing this issue is available here https://youtube.com/watch?v=vrCuCnHYep0 :)

If someone would like to work on it, please let me know and I'll do anything I can to ease the process 🎉

In the clippy.rs file, there's a useless .clone() call at the end of the Linter's clippy function.

We should switch

println!("{}", String::from_utf8(build.stdout.clone())?);

to

println!("{}", String::from_utf8(build.stdout)?);

This would be a great way to make a first contribution, and learn how to fork a project and make your first pull request.

If anyone has a question, please let me know and I'll gladly help ! :)

retrieve --features "foo bar baz" argument and pass it to clippy

A live stream on how to fix #11, which almost requires the same steps as fixing this issue is available here https://youtube.com/watch?v=vrCuCnHYep0 :)

If someone would like to work on it, please let me know and I'll do anything I can to ease the process 🎉

Right now, we run the linter on each workspace member, which might not be required.

If workspace foo has members bar and baz, and the git diff shows there are only changes in member bar, there is no need to run the linter on member baz

Such a change would probably have to be done in scout.rs, where we could filter relevant members before we iterate

Update docs for public functions that return Result: more on this in clippy's missing_errors_doc.
Remove #[allow(clippy::missing_errors_doc)] attribute.

cargo-scout does not display errors when the binary is run twice without making any changes to the code.

Steps to reproduce:

1. Violate clippy::pedantic rule. (e.g., create a 100 LoC function).
2. Run the program. (It displays the errors correctly)
3. Run the program again without making any changes.

This results in: No warnings raised by clippy::pedantic in your diff, you're good to go!. I suppose the expected behavior should be that the program should output the warnings again.

In order to enable the correct features for each subcrate, cargo-scout should be able to get a list of available features, and intersect with a list of enabled features to run clippy with the correct set (and not add any feature that is not available)

Examples:

Example 1

crate has a feature named bar
cargo-scout is invoked as cargo-scout --features "foo bar"

command run

clippy should be run with the bar feature only.

Example 2

crate is in a workspace, has first subcrate named foo, a second named bar, and both define a feature named baz.

command run

cargo scout is invoked ad cargo-scout is invoked as cargo-scout --features "baz"

Both clippy commands must add the feature baz.

command run

cargo-scout is invoked as cargo-scout --features "foo/baz"

the feature baz should only be added to the clippy check on subcrate foo.