[engine] model snapshots in validation, make root rules a dict instead of a set #4125

Merged
merged 8 commits into from Dec 7, 2016

Conversation

Projects
None yet
2 participants
@baroquebobcat
Contributor

baroquebobcat commented Dec 6, 2016

Problem

I'm working on porting my patch for #3580 to the native engine, (#3960). First, I'm updating the graph construction code with the changes there. It hadn't covered Snapshot processes, and it modeled root rules as a set rather than a graph, which caused problems.

Solution

Include snapshot process' behavior in the rule graph construction used by the validator. Update tests to expect the new visualization format.

List of changes

  • rename NodeBuilder to RuleIndex
  • rename RootRule to RootRuleGraphEntry
  • support SnapshotProcess in validator
  • Name snapshot process funcs so they are printable
  • Include subject and product types in intrinsic rule repr
  • Include graph generation methods that can construct new graphs from an existing one--these are currently unused
  • Raise an error if a declared rule is not reachable from the declared root types.
  • Unify naming for input selector for projection selectors
  • If field of a SelectDependencies is the default value, don't include it in the repr
  • Include a map of root rule to the rules that would fulfill it.
@stuhood

stuhood approved these changes Dec 7, 2016

Thanks Nick.

@@ -97,11 +97,15 @@ class SelectDependencies(datatype('Dependencies',
`dep_product`.
"""
- def __new__(cls, product, dep_product, field='dependencies', field_types=tuple(), transitive=False):
+ DEFAULT_FIELD = 'dependencies'

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

This is the default field only for the Dependencies selector, yea?

@stuhood

stuhood Dec 7, 2016

Member

This is the default field only for the Dependencies selector, yea?

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Yes. I wanted to have a variable for it so that both places would reference the same thing, rather than using literals in both places. But I can inline them if you think it's not worth the additional complexity.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Yes. I wanted to have a variable for it so that both places would reference the same thing, rather than using literals in both places. But I can inline them if you think it's not worth the additional complexity.

+ if root_rule in self.root_rules:
+ return root_rule
+
+ def new_graph_with_root_for(self, subject_type, selector):

This comment has been minimized.

@stuhood

stuhood Dec 7, 2016

Member

Unused I think?

@stuhood

stuhood Dec 7, 2016

Member

Unused I think?

This comment has been minimized.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Yeah. I noted that in the description, but I don't think it was sufficiently called out. I could just rm them since I'll be reimplementing in rust anyway.

@baroquebobcat

baroquebobcat Dec 7, 2016

Contributor

Yeah. I noted that in the description, but I don't think it was sufficiently called out. I could just rm them since I'll be reimplementing in rust anyway.

@baroquebobcat baroquebobcat merged commit 0dd2423 into pantsbuild:master Dec 7, 2016

1 check passed

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

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[engine] model snapshots in validation, make root rules a dict instea…
…d of a set (#4125)

### Problem
I'm working on porting my patch for #3580 to the native engine, (#3960). First, I'm updating the graph construction code with the changes there. It hadn't covered Snapshot processes, and it modeled root rules as a set rather than a graph, which caused problems.

### Solution

Include snapshot process' behavior in the rule graph construction used by the validator. Update tests to expect the new visualization format.

### List of changes
* rename NodeBuilder to RuleIndex
* rename RootRule to RootRuleGraphEntry
* support SnapshotProcess in validator
* Name snapshot process funcs so they are printable
* Include subject and product types in intrinsic rule repr
* Include graph generation methods that can construct new graphs from an existing one--these are currently unused
* Raise an error if a declared rule is not reachable from the declared root types.
* Unify naming for input selector for projection selectors
* If field of a SelectDependencies is the default value, don't include it in the repr
* Include a map of root rule to the rules that would fulfill it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment