Split the crates into a bin and a lib, within the cargo-scout wor… #61

o0Ignition0o · 2019-12-20T21:35:49Z

No description provided.

o0Ignition0o · 2019-12-20T21:42:18Z

Isht there's a lot going on here, but it should be a good step towards #56 and #59

o0Ignition0o · 2019-12-20T23:16:59Z

@ebroto | @daubaris it's a pretty huge one, but the split starts looking ok. Plus they allow me to figure out exactly what to expose, and start writing docs.
I'm probably going to merge it tomorrow or on monday, let me know if you have ideas / comments :)

o0Ignition0o · 2019-12-21T10:02:28Z

Here's what the documentation looks like so far:

ebroto · 2019-12-21T10:07:21Z

Looking good! If it's OK I'll take a look later in the day when I have some free time :)

o0Ignition0o · 2019-12-21T10:25:53Z

Sure no problem, take your time :)

ebroto

Awesome work! I think this is a great improvement regarding separation of concerns. The code grew a bit in complexity, but I think it's worth it to accomodate the possibility of being more general than just running clippy on rust files. Also, having documentation now makes the code more accessible 👍

One last remark, the src/ directory in the root should be removed.

ebroto · 2019-12-21T20:37:35Z

cargo-scout-lib/src/config/rust.rs

+}
+
+impl CargoConfig {
+    /// This function will instanciate a Config from a Cargo.toml path.


I think it should be "instantiate"

ebroto · 2019-12-21T20:37:55Z

cargo-scout-lib/src/config/rust.rs

+use crate::config::Config;
+use serde::Deserialize;
+
+#[derive(Deserialize, PartialEq, Debug, Clone)]


Deserialize, PartialEq and Clone seem unused. Maybe it's done for forward compatibility?

ebroto · 2019-12-21T20:39:44Z

cargo-scout-lib/src/config/rust.rs

+    /// assert_eq!(vec!["cargo-scout".to_string(), "cargo-scout-lib".to_string()], config.get_members());
+    /// # Ok::<(), cargo_scout_lib::error::Error>(())
+    /// ```
+    pub fn from_manifest_path(p: impl AsRef<std::path::Path>) -> Result<Self, crate::error::Error> {


Why not importing Path? Same for Manifest. For the error, I think cargo_scout_lib::Error would be more idiomatic (like std does for io::Error within io).

As explained in this pretty cool blog post AsRef allows the function to accept anything that allows a Path to be borrowed from it. So it allows us to automagically get a Path from a String, which I think is pretty cool !

pub use error::Error; is a great idea !

That's cool indeed!

I meant why don't we do

use std::path::Path; ... AsRef<Path>

Instead of using the fully qualified path. Just for consistency with other cases

Oh sorry I didn’t understand that’s what you meant!
Since it’s fairly easy to accomplish, and it’s not that critical, let’s mark it as a good first pull request, so someone new to rust and open source might be able to grab it !

ebroto · 2019-12-21T20:45:02Z

cargo-scout-lib/src/config/mod.rs

+    ///    }
+    /// }
+    /// ```
+    fn get_members(&self) -> Vec<String>;


According to the API guidelines, the get_ prefix should be avoided.

nice catch, TIL ! <3

ebroto · 2019-12-21T20:47:28Z

cargo-scout-lib/src/vcs/git.rs

+    where
+        P: AsRef<Path>,
+    {
+        println!("[VCS] - Getting diff with target {}", &self.target_branch);


I think we should not write logs from the lib, at least not unconditionally.

I think you're right, I put it there until we can work on #47 and have a better solution alltogether

ebroto · 2019-12-21T20:49:57Z

cargo-scout-lib/src/linter/mod.rs

-pub struct LintCode {
-    pub code: String,
-    pub explanation: String,
+struct LintCode {


LintCode and LintSpan seem unused if I'm not mistaken.

Oh that was a leftover, nice catch ! :)

ebroto · 2019-12-21T20:52:42Z

cargo-scout-lib/src/linter/clippy.rs

@@ -12,21 +13,16 @@ pub struct Clippy {

 impl Linter for Clippy {
    fn get_lints(&self, working_dir: PathBuf) -> Result<Vec<Lint>, crate::error::Error> {


Same remark as for the get_members method in the Config trait.

ebroto · 2019-12-21T20:55:49Z

cargo-scout-lib/Cargo.toml

@@ -0,0 +1,25 @@
+[package]
+name = "cargo-scout-lib"


Maybe it's personal opinion, but I find cargo_scout_lib to be too long and a bit redundant when used in the binary. What do you think about naming the lib scout, and the binary cargo-scout ?

I wanted to do it as well, but it turns out there's already a fuzzer out there that goes by the name scout, and I'm not really sure renaming the lib scout-lib wouldn't confuse people more :/
https://crates.io/search?q=scout
Maybe we can find an other name ?

Oh I see, OK then, finding a name is not easy task so let's go with this for now :)

ebroto · 2019-12-21T21:35:06Z

cargo-scout-lib/src/linter/mod.rs

    /// The line where the lint should be reported
    ///
    /// GitHub provides a line_start and a line_end.
    /// We should use the line_start in case of multi-line lints.
    /// (Why?)
-    pub line_start: usize,
+    line_start: usize,
 }

 #[derive(Deserialize, PartialEq, Debug, Clone)]


Since the latest stable, we can mark all pub structs with pub fields #[non_exhaustive]

hmm that's interesting. In the readme I claimed the MSRV was 1.37 but I suppose we can bump it to 1.40 and do it in a subsequent PR.
It would also allow us to slap non_exhaustive to our error enum, just in case :D

o0Ignition0o force-pushed the lib_bin_split branch from d63027f to 4283b7f Compare December 20, 2019 21:41

o0Ignition0o force-pushed the lib_bin_split branch from 4283b7f to 4349641 Compare December 20, 2019 22:09

Split the crates into a bin and a lib, within the cargo-scout workspace.

b700bde

o0Ignition0o force-pushed the lib_bin_split branch from d6aadc8 to b494b19 Compare December 20, 2019 23:15

Config now only returns members, which will always be iterated on.

f193542

o0Ignition0o force-pushed the lib_bin_split branch from b494b19 to 63a1470 Compare December 20, 2019 23:25

o0Ignition0o changed the title ~~Split the crates into a bin and s lib, within the cargo-scout workspace.~~ Split the crates into a bin and a lib, within the cargo-scout workspace. Dec 20, 2019

o0Ignition0o force-pushed the lib_bin_split branch from 63a1470 to 42ccd2c Compare December 21, 2019 10:01

o0Ignition0o force-pushed the lib_bin_split branch from 42ccd2c to 90fa141 Compare December 21, 2019 10:03

o0Ignition0o force-pushed the lib_bin_split branch from 90fa141 to ce571d1 Compare December 21, 2019 13:31

Config documentation.

4bd8327

o0Ignition0o force-pushed the lib_bin_split branch from ce571d1 to 4bd8327 Compare December 21, 2019 13:32

Linter documentation.

f179400

ebroto suggested changes Dec 21, 2019

View reviewed changes

ebroto reviewed Dec 21, 2019

View reviewed changes

Fixups before merge

85f4a36

o0Ignition0o mentioned this pull request Dec 21, 2019

Spot convenient uses of #[non_exhaustive] in the workspace #62

Open

o0Ignition0o changed the title ~~Split the crates into a bin and a lib, within the cargo-scout workspace.~~ Split the crates into a bin and a lib, within the cargo-scout wor… Dec 21, 2019

o0Ignition0o merged commit e95867b into master Dec 21, 2019

o0Ignition0o deleted the lib_bin_split branch December 21, 2019 22:35

o0Ignition0o mentioned this pull request Dec 22, 2019

use std::path::Path #63

Closed

o0Ignition0o mentioned this pull request Jan 12, 2020

Bikeshed Scout Options #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the crates into a bin and a lib, within the cargo-scout wor… #61

Split the crates into a bin and a lib, within the cargo-scout wor… #61

o0Ignition0o commented Dec 20, 2019

o0Ignition0o commented Dec 20, 2019 •

edited

Loading

o0Ignition0o commented Dec 20, 2019

o0Ignition0o commented Dec 21, 2019

ebroto commented Dec 21, 2019

o0Ignition0o commented Dec 21, 2019

ebroto left a comment

ebroto Dec 21, 2019

ebroto Dec 21, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019 •

edited

Loading

ebroto Dec 22, 2019 •

edited

Loading

o0Ignition0o Dec 22, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019

ebroto Dec 21, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019

ebroto Dec 22, 2019

ebroto Dec 21, 2019

o0Ignition0o Dec 21, 2019

		@@ -12,21 +13,16 @@ pub struct Clippy {

		impl Linter for Clippy {
		fn get_lints(&self, working_dir: PathBuf) -> Result<Vec<Lint>, crate::error::Error> {

Split the crates into a bin and a lib, within the cargo-scout wor… #61

Split the crates into a bin and a lib, within the cargo-scout wor… #61

Conversation

o0Ignition0o commented Dec 20, 2019

o0Ignition0o commented Dec 20, 2019 • edited Loading

o0Ignition0o commented Dec 20, 2019

o0Ignition0o commented Dec 21, 2019

ebroto commented Dec 21, 2019

o0Ignition0o commented Dec 21, 2019

ebroto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o Dec 21, 2019 • edited Loading

Choose a reason for hiding this comment

ebroto Dec 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o commented Dec 20, 2019 •

edited

Loading

o0Ignition0o Dec 21, 2019 •

edited

Loading

ebroto Dec 22, 2019 •

edited

Loading