o0ignition0o / cargo-scout Goto Github PK
View Code? Open in Web Editor NEWLicense: Other
License: Other
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.
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 !
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 ! 🎉
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)
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.
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
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.
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
As mentionned here
Let's try to play around with the cargo crate or the clippy crate and see what can be done.
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 ?
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:
crate has a feature named bar
cargo-scout is invoked as cargo-scout --features "foo bar"
clippy should be run with the bar feature only.
crate is in a workspace, has first subcrate named foo, a second named bar, and both define a feature named baz.
cargo scout is invoked ad cargo-scout is invoked as cargo-scout --features "baz"
Both clippy commands must add the feature baz.
cargo-scout is invoked as cargo-scout --features "foo/baz"
the feature baz should only be added to the clippy check on subcrate foo.
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.
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 🎉
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.
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
the --no-default-features flag should be caught and passed to clippy
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.
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 ! :)
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 :)
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 :)
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.
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.)
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 :)
Update docs for public functions that return Result: more on this in clippy's missing_errors_doc.
Remove #[allow(clippy::missing_errors_doc)]
attribute.
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
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 🎉
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!
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.
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
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>
Needs to be investigated, it's probably clippy related
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.
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
As you can see in our already existing tests
there are 3 Phases:
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:
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 :)
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 ?
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.
The options keep adding up, and one parser has a constructor, the other one doesnt. let's use a builder pattern instead !
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 :)
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 ^^
The project is getting bigger and it would be a good idea to split separate parts (tests in this case) into a separate location.
cargo-scout
does not display errors when the binary is run twice without making any changes to the code.
Steps to reproduce:
clippy::pedantic
rule. (e.g., create a 100 LoC function).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.
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
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.