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

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

Merged
merged 5 commits into from
Dec 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 4 additions & 25 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,26 +1,5 @@
[package]
name = "cargo-scout"
version = "0.4.0"
authors = ["o0Ignition0o <jeremy.lempereur@gmail.com>"]
description = "Run clippy::pedantic on your diffs only"
homepage = "https://github.com/o0Ignition0o/cargo-scout"
repository = "https://github.com/o0Ignition0o/cargo-scout"
keywords = ["clippy", "lint","pedantic", "cli", "diff"]
license = "MIT/Apache-2.0"
include = [
"**/*.rs",
"Cargo.toml",
[workspace]
members = [
"cargo-scout",
"cargo-scout-lib",
]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
serde = { version = "1.0.103", features = ["derive"] }
serde_json = "1.0.44"
structopt = "0.3.5"
cargo_toml = "0.8.0"
git2 = { version = "0.11.0", default-features = false }

[dev-dependencies]
tempfile = "3.1.0"
25 changes: 25 additions & 0 deletions cargo-scout-lib/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
[package]
name = "cargo-scout-lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

version = "0.5.0"
authors = ["o0Ignition0o <jeremy.lempereur@gmail.com>"]
description = "Lib that powers cargo-scout, and allows you to run / implement your own linters"
homepage = "https://github.com/o0Ignition0o/cargo-scout"
repository = "https://github.com/o0Ignition0o/cargo-scout"
keywords = ["clippy", "lint","pedantic", "cli", "diff"]
license = "MIT/Apache-2.0"
include = [
"**/*.rs",
"Cargo.toml",
]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
cargo_toml = "0.8.0"
git2 = { version = "0.11.*", default-features = false }
serde = { version = "1.0.*", features = ["derive"] }
serde_json = "1.0.*"

[dev-dependencies]
tempfile = "3.1.0"
78 changes: 78 additions & 0 deletions cargo-scout-lib/src/config/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
pub mod rust;

/// This trait is responsible for providing a list of members,
/// which are directories to be linted against.
pub trait Config {
/// This function should return a list of relative paths
/// a linter will iterate on.
///
/// If only the current working directory must be checked, it must return `vec![".".to_string()]`
///
/// If several directories must be checked,
/// return their relative path as strings.
///
/// For example if your current working directory is `foo`,
/// and you want to check `./bar` and `./baz`,
/// return `vec!["bar".to_string(), "baz".to_string()]`
///
/// # Example with the root directory
/// ```
/// # use cargo_scout_lib::config::Config;
/// # struct CustomConfig{}
/// # impl CustomConfig {
/// # fn new() -> Self {
/// # Self {}
/// # }
/// # }
/// # // Your own implementation goes here
/// # impl Config for CustomConfig {
/// # fn members(&self) -> Vec<String> {
/// # vec![".".to_string()]
/// # }
/// # }
/// let config = CustomConfig::new();
/// // Only the current directory must be linted
/// assert_eq!(vec![".".to_string()], config.members());
/// ```
///
/// # Example with two subdirectories
/// ```
/// # use cargo_scout_lib::config::Config;
/// # struct CustomConfig{}
/// # impl CustomConfig {
/// # fn new() -> Self {
/// # Self {}
/// # }
/// # }
/// # // Your own implementation goes here
/// # impl Config for CustomConfig {
/// # fn members(&self) -> Vec<String> {
/// # vec!["foo".to_string(), "bar".to_string()]
/// # }
/// # }
/// let config = CustomConfig::new();
/// // Directories ./foo and ./bar must be linted
/// assert_eq!(vec!["foo".to_string(), "bar".to_string()], config.members());
/// ```
///
/// # Implementing your own Config
/// ```
/// use cargo_scout_lib::config::Config;
///
/// struct CustomConfig{}
///
/// # impl CustomConfig {
/// # fn new() -> Self {
/// # Self {}
/// # }
/// # }
/// impl Config for CustomConfig {
/// fn members(&self) -> Vec<String> {
/// // Your own code to fetch the list of
/// // directories to iterate on goes here
/// # vec![".".to_string()]
/// }
/// }
/// ```
fn members(&self) -> Vec<String>;
}
110 changes: 110 additions & 0 deletions cargo-scout-lib/src/config/rust.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
use crate::config::Config;

/// This struct represents a Cargo project configuration.
pub struct CargoConfig {
members: Vec<String>,
}

impl Config for CargoConfig {
#[must_use]
fn members(&self) -> Vec<String> {
self.members.clone()
}
}

impl CargoConfig {
/// This function will instantiate a Config from a Cargo.toml path.
///
/// If in a workspace, `get_members` will return the members
/// of the [[workspace]] members section in Cargo.toml.
///
/// Else, it will return `vec![".".to_string()]`
///
/// # cargo-scout-lib example
/// ```
/// # use cargo_scout_lib::config::Config;
/// # use cargo_scout_lib::config::rust::CargoConfig;
/// let config = CargoConfig::from_manifest_path("Cargo.toml")?;
/// // There is only one directory to lint, which is the current one.
/// assert_eq!(vec!["."], config.members());
/// # Ok::<(), cargo_scout_lib::Error>(())
/// ```
///
/// # cargo-scout workspace example
/// ```
/// # use cargo_scout_lib::config::Config;
/// # use cargo_scout_lib::config::rust::CargoConfig;
/// let config = CargoConfig::from_manifest_path("../Cargo.toml")?;
/// // We will lint `./cargo-scout` and `./cargo-scout-lib`.
/// assert_eq!(vec!["cargo-scout".to_string(), "cargo-scout-lib".to_string()], config.members());
/// # Ok::<(), cargo_scout_lib::Error>(())
/// ```
pub fn from_manifest_path(p: impl AsRef<std::path::Path>) -> Result<Self, crate::error::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Owner Author

@o0Ignition0o o0Ignition0o Dec 21, 2019

Choose a reason for hiding this comment

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

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 !

Copy link
Contributor

@ebroto ebroto Dec 22, 2019

Choose a reason for hiding this comment

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

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

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 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 !

Ok(Self::from_manifest(cargo_toml::Manifest::from_path(p)?))
}

fn from_manifest(m: cargo_toml::Manifest) -> Self {
if let Some(w) = m.workspace {
Self { members: w.members }
} else {
Self {
// Project root only
members: vec![".".to_string()],
}
}
}
}

#[cfg(test)]
mod tests {
use crate::config::rust::CargoConfig;
use crate::config::Config;

#[test]
fn test_not_workspace_manifest() {
let manifest = cargo_toml::Manifest::from_path("Cargo.toml").unwrap();
// Make sure we actually parsed the manifest
assert_eq!("cargo-scout-lib", manifest.clone().package.unwrap().name);
let config = CargoConfig::from_manifest(manifest);
assert_eq!(vec!["."], config.members());
}
#[test]
fn test_not_workspace_path() {
let config = CargoConfig::from_manifest_path("Cargo.toml").unwrap();
assert_eq!(vec!["."], config.members());
}
#[test]
fn test_neqo_members_manifest() {
let neqo_toml = r#"[workspace]
members = [
"neqo-client",
"neqo-common",
"neqo-crypto",
"neqo-http3",
"neqo-http3-server",
"neqo-qpack",
"neqo-server",
"neqo-transport",
"neqo-interop",
"test-fixture",
]"#;

let manifest = cargo_toml::Manifest::from_slice(neqo_toml.as_bytes()).unwrap();
let config = CargoConfig::from_manifest(manifest);
assert_eq!(
vec![
"neqo-client",
"neqo-common",
"neqo-crypto",
"neqo-http3",
"neqo-http3-server",
"neqo-qpack",
"neqo-server",
"neqo-transport",
"neqo-interop",
"test-fixture"
],
config.members()
);
}
}
6 changes: 6 additions & 0 deletions src/error.rs → cargo-scout-lib/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,42 @@ pub enum Error {
}

impl From<std::string::FromUtf8Error> for Error {
#[must_use]
fn from(err: std::string::FromUtf8Error) -> Self {
Self::Utf8(err)
}
}

impl From<serde_json::Error> for Error {
#[must_use]
fn from(err: serde_json::Error) -> Self {
Self::Json(err)
}
}

impl From<String> for Error {
#[must_use]
fn from(err: String) -> Self {
Self::Command(err)
}
}

impl From<std::io::Error> for Error {
#[must_use]
fn from(err: std::io::Error) -> Self {
Self::Io(err)
}
}

impl From<cargo_toml::Error> for Error {
#[must_use]
fn from(err: cargo_toml::Error) -> Self {
Self::CargoToml(err)
}
}

impl From<git2::Error> for Error {
#[must_use]
fn from(err: git2::Error) -> Self {
Self::Git(err)
}
Expand Down
7 changes: 7 additions & 0 deletions cargo-scout-lib/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
pub mod config;
pub mod error;
pub mod linter;
pub mod scout;
pub mod vcs;

pub use error::Error;
Loading