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

ISSUE-16 - Further unit testing #129

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

jmcconnell26
Copy link
Contributor

No description provided.

@anderejd
Copy link
Contributor

Thanks for chipping away at the test suite 👍

Are the formatting changes done by cargo fmt?

@jmcconnell26
Copy link
Contributor Author

Hi,
No worries! I just want to be sure that the changes I make for removing the cargo crate don't break anything!
And yeah, the changes to the formatting are just cargo fmt.
Do you have a particular view on using it?

I was also wondering, would you mind if I raise a PR in https://github.com/rust-unofficial/awesome-rust mentioning cargo-geiger? I found it really useful when I was getting started in rust.

Thanks,
Josh

@anderejd
Copy link
Contributor

And yeah, the changes to the formatting are just cargo fmt.
Do you have a particular view on using it?

I love code formatting tools in general and cargo fmt / rustfmt is great!

I was also wondering, would you mind if I raise a PR in https://github.com/rust-unofficial/awesome-rust mentioning cargo-geiger? I found it really useful when I was getting started in rust.

Sounds good to me 👍

@tarcieri
Copy link
Collaborator

@anderejd if you'd like I can add a CI check for rustfmt

@anderejd
Copy link
Contributor

That would be perfect!

Comment on lines 203 to 225
fn create_package_id_vec(count: i32) -> Vec<PackageId> {
let config = Config::default().unwrap();

let current_working_dir =
env::current_dir().unwrap().join("Cargo.toml");

let manifest_path_option = Some(current_working_dir);

let workspace = get_workspace(&config, manifest_path_option).unwrap();

let package = workspace.current().unwrap();

let source_id = package.dependencies().first().unwrap().source_id();

let mut package_id_vec: Vec<PackageId> = vec![];

for i in 0..count {
package_id_vec.push(
PackageId::new(
format!("test_name_{}", i),
format!("1.2.{}", i).as_str(),
source_id,
)
.unwrap(),
)
}

package_id_vec
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, formatting: This function would be easier to read imho without any extra empty lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout! Have pushed an update.

For the use of cargo format would it also be worth drafting a contributing guideline as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, a short contributing section in the readme file could be useful.

ISSUE-16 - Adding in further unit tests:
* Pulling up unit test coverage to just under 70%
* Splitting out traversal module into submodules
* Making cargo clippy suggestions
* Running cargo fmt

Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
* Remove extra line spacing

Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
@anderejd
Copy link
Contributor

Sorry for the conflict, I just pushed a cargo fmt to master.

@jmcconnell26
Copy link
Contributor Author

No worries! Have merged

@anderejd anderejd merged commit 849dcbe into geiger-rs:master Oct 26, 2020
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.

3 participants