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 - Refactor cli module #111

Merged
merged 2 commits into from
Sep 2, 2020

Conversation

jmcconnell26
Copy link
Contributor

Split cli module into component parts. Create:

  • find module
  • format - table module
  • format - tree module
  • rs_file module
  • scan module
  • traversal module

Signed-off-by: Josh McConnell jmcconnell26@qub.ac.uk

Split cli module into component parts. Create:

* find module
* format - table module
* format - tree module
* rs_file module
* scan module
* traversal module

Signed-off-by: Josh McConnell <jmcconnell26@qub.ac.uk>
@anderejd
Copy link
Contributor

anderejd commented Sep 2, 2020

At a first glance this looks like a great improvement! Thanks!
I would like to take some time to study the changes later today before merging.

use std::sync::Arc;
use std::sync::Mutex;
use walkdir::DirEntry;
use walkdir::WalkDir;

// ---------- BEGIN: Public items ----------
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove these comments now when modules are much smaller. I haven't seen similar comment sections in other Rust projects so unless you insist to keep them let's remove them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a good shout, I had kept them in to keep a consistent style, but am happy to take them out.

Comment on lines 105 to 110
pub enum DetectionStatus {
NoneDetectedForbidsUnsafe,
NoneDetectedAllowsUnsafe,
UnsafeDetected,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this type should probably be renamed to something like CrateDetectionStatus and moved back to the cargo-geiger crate since the geiger library doesn't know about crates, only files.

@jmcconnell26
Copy link
Contributor Author

Thanks for the review!
I'll update the PR with the changes now

@jmcconnell26
Copy link
Contributor Author

I have just made the changes, would they be better in a separate commit, or would you rather I amend the first?

@anderejd
Copy link
Contributor

anderejd commented Sep 2, 2020

I have just made the changes, would they be better in a separate commit, or would you rather I amend the first?

A new commit can be easier to review sometimes.

Make the following changes:
* Remove all "BEGIN: Public items" comments
* Move DetectionStatus enum back to cargo-geiger and rename to
  CrateDetectionStatus

Signed-off-by: Josh McConnell <jmcconnell26@qub.ac.uk>
@anderejd anderejd merged commit ac71566 into geiger-rs:master Sep 2, 2020
@anderejd
Copy link
Contributor

anderejd commented Sep 2, 2020

Good work, thanks again!

@jmcconnell26 jmcconnell26 deleted the ISSUE-16-Refactoring branch September 28, 2020 17:31
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.

2 participants