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

Extract RuleGraph into a separate crate #7767

Merged
merged 7 commits into from May 22, 2019

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

commented May 20, 2019

Problem

The RuleGraph is relatively intertwined with the rest of the engine and has dependencies on the python code and the externs. This makes it more challenging to test, and its interface less clear.

Solution

Extract implementations of Rule and DependencyKey from the RuleGraph, then move RuleGraph to its own crate and parameterize it on a R: Rule type.

Result

Adding tests for #7710 should be more straightforward.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Commits are useful to review independently.

@Eric-Arellano
Copy link
Contributor

left a comment

Looks like a good improvement to me. Thanks!

Although probably best for a review from someone with more Rust experience. Disclaimer that I understood the last commit the least.

.iter()
.filter(|r| !reachable_rules.contains(r))
.map(|&r| UnreachableError::new(r.clone()))
Itertools::flatten(self.tasks.values().map(|r| r.iter()))

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Contributor

Could you use flat map? It looks like stdlib defines it: https://doc.rust-lang.org/std/iter/struct.FlatMap.html.

This comment has been minimized.

Copy link
@stuhood

stuhood May 22, 2019

Author Member

Not only can I use flat_map, it looks like flatten is built in now. Cool.

pub enum Rule {
// Intrinsic rules are implemented in rust.
Intrinsic(Intrinsic),
// Task rules are implemented in python.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Contributor

Nit: capitalize language names.

// Used during the construction of the tasks map.
preparing: Option<Task>,
}

///
/// A collection of Rules (TODO: rename to Rules).

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Contributor

This is confusing. I think it would make more sense if you use backticks:

/// A collection of `Rule`s (TODO: rename to `Rules`).
let satisfiable_entries = input_entries
.iter()
.filter(|input_entry| {
input_entry
.params()
.iter()
.all(|p| available_params.contains(p) || Some(p) == provided_param)
.all(|p| available_params.contains(p) || Some(*p) == provided_param)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Contributor

Rust will dereference automatically, right? This is being done as a matter of explicitness?

This comment has been minimized.

Copy link
@stuhood

stuhood May 22, 2019

Author Member

In many cases it will, yea. I think that in this case the dereference is necessary in order to construct the right type of option for the comparison (ie, the auto-deref doesn't kick in, because it's totally valid to have an Option<&TypeId>... it's just not what we want here.)

),
&EntryWithDeps::Root(ref root) => {
// TODO: Consider dropping this (final) public use of the keyword "Select", while ensuring
// that error messages remain sufficiently grokkable.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 20, 2019

Contributor

I'd arguable that they are less understandable because of Select being included. I started using V2 after Select was already dropped, so it's Greek to me.

This comment has been minimized.

Copy link
@stuhood

stuhood May 22, 2019

Author Member

Yep. This just preserves the status quo.

@stuhood stuhood force-pushed the twitter:stuhood/extract-rule-graph-crate branch from dc3fb56 to 0286f1c May 22, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Rebased, and added two commits for 1) the first two basic tests, 2) review feedback.

Will merge on green.

@stuhood stuhood merged commit 6b68f90 into pantsbuild:master May 22, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@stuhood stuhood deleted the twitter:stuhood/extract-rule-graph-crate branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.