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 - Remove Cargo Core from Traversal Module: #135

Merged

Conversation

jmcconnell26
Copy link
Contributor

  • 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

Signed-off-by: joshmc josh-mcc@tiscali.co.uk

@jmcconnell26 jmcconnell26 force-pushed the ISSUE-16-RemoveCargoCoreTraversal branch from 2b8f74d to 5692aa3 Compare November 5, 2020 19:56
@jmcconnell26
Copy link
Contributor Author

A few small clean ups, and a very small change to pull the use of the cargo core PackageId out of the traversal module

Comment on lines 107 to 123
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)?;
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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>
@jmcconnell26 jmcconnell26 force-pushed the ISSUE-16-RemoveCargoCoreTraversal branch from 5692aa3 to c728d04 Compare November 6, 2020 12:19
@anderejd anderejd merged commit 3c95ba5 into geiger-rs:master Nov 6, 2020
@jmcconnell26 jmcconnell26 deleted the ISSUE-16-RemoveCargoCoreTraversal branch November 6, 2020 15:57
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.

None yet

2 participants