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

Remove @rule Select syntax #7477

Merged
merged 4 commits into from
Apr 2, 2019
Merged

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Apr 2, 2019

Problem

The Select syntax in @rule declarations used to differentiate the declarative parameter selectors, but with the advent of coroutine @rules and removal of TypeConstraint, the Select syntax no longer has any use.

Solution

Remove the Select syntax for @rules, while preserving it as the marker for root selections in the graph. Update the documentation for @rules to refer to these as "parameter selectors".

Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing the work to make this change across the codebase and to update the docs! I really, really like the way declaring @rules looks like now!!

(`Select(IntType)`), this `@rule` represents a conversion from `IntType` to `StrType`, with no
second argument is a list of parameter selectors that declare the types of the input parameters for
the `@rule`. In this case, because the Product type is `StringType` and there is one parameter
selector (for `IntType`), this `@rule` represents a conversion from `IntType` to `StrType`, with no
Copy link
Contributor

Choose a reason for hiding this comment

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

"parameter selectors" is a very good edit, this leaves the opportunity to expand it later as needed while making it extremely clear what those types are doing there.

src/python/pants/engine/rules.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Cool! This is a much nicer syntax.

Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This is a great clean-up, thanks!

pub fn select_str(select: &Select) -> String {
format!("Select({})", select.product).to_string() // TODO variant key
pub fn select_root_str(select: &Select) -> String {
format!("Select({})", select.product)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename Select to Root or SelectRoot or something here, now that that's what it represents?

Copy link
Member Author

Choose a reason for hiding this comment

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

"It's complicated", because Select is still the Node that we run to kick off a request for the parameters to a rule that is going to run, and in that case it isn't a root.

@stuhood stuhood merged commit 76415e5 into pantsbuild:master Apr 2, 2019
@stuhood stuhood deleted the stuhood/remove-select branch April 2, 2019 16:17
@ity
Copy link
Contributor

ity commented Apr 3, 2019

nice change!

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.

5 participants