-
Notifications
You must be signed in to change notification settings - Fork 65
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 - Remove Cargo Core from Traversal Module: #135
ISSUE-16 - Remove Cargo Core from Traversal Module: #135
Conversation
2b8f74d
to
5692aa3
Compare
A few small clean ups, and a very small change to pull the use of the cargo core |
cargo-geiger/src/cli.rs
Outdated
pub fn resolve<'a, 'cfg>( | ||
args: &Args, | ||
package_id: PackageId, | ||
registry: &mut PackageRegistry<'cfg>, | ||
workspace: &'a Workspace<'cfg>, | ||
features: &[String], | ||
all_features: bool, | ||
no_default_features: bool, | ||
) -> CargoResult<(PackageSet<'a>, Resolve)> { | ||
let dev_deps = true; // TODO: Review this. | ||
let uses_default_features = !no_default_features; | ||
let uses_default_features = !args.no_default_features; | ||
let opts = ResolveOpts::new( | ||
dev_deps, | ||
features, | ||
all_features, | ||
&args.features.clone(), | ||
args.all_features, | ||
uses_default_features, | ||
); | ||
let prev = ops::load_pkg_lockfile(workspace)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to bundle up some of the parameters into a struct here, however Args
provides around 30 members of which only 3 are needed by this function. This is probably of lesser importance but I would like us to create a new struct for this purpose, to avoid introducing tight coupling between all of the command line arguments and this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anderejd, that's a good shout, we definitely don't need to be throwing the whole Args
struct about.
I've updated the PR to pull out a struct just for these values, which should allow a decoupling from the remainder of the command line arguments.
Please let me know if you had something different in mind?
Thanks,
Josh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* Switch Args struct to use Default * Refactor rs_file module to under scan * Remove Node struct from Graph struct, instead only use PackageId * Pull out function to parse features from args * Pull out cargo core usage from traversal module * Create FeatureArgs struct within Args Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
5692aa3
to
c728d04
Compare
Signed-off-by: joshmc josh-mcc@tiscali.co.uk