Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add generic mechanism to codegen sources in V2 #9634
Add generic mechanism to codegen sources in V2 #9634
Changes from 2 commits
6e7e1b7
7877e89
5340f74
ff97ac4
9fe244a
d76cbfc
bdbb774
3e64493
3596a9c
27af0c8
b29c30c
4138f38
e05d8b6
7aabe19
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This rule probably warrants a docstring that incorporates some of the description from this PR?
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.
There's already a lot of documentation now in the rule body and in the docstring of the relevant classes. I think adding docstring to the rule might be noisy.
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.
This should probably not be a
RootRule
: the conventions are nowhere near solid, but I think of them as being a bit like a public API, and in this caseGenerateSourcesRequest
is an implementation detail ofHydrateSourcesRequest
.If a test wants to poke at those rules more directly, it can add a
RootRule
itself to do so. But I think that we should think of the roots that we expose in rulesets as their external inputs for general use.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.
Hm, my understanding of RootRules is that you should use them whenever you directly inject a type into the graph, rather than deriving that type from other rules in the graph? Meaning, almost every
Request
class should be aRootRule
because they are almost always directly created through a Python constructor, rather than being derived from some other rule.Is this mental model the wrong way of understanding
RootRule
?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.
Yes, that is the wrong way to think about
RootRule
. See https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/README.md#gets-and-rootrulesIn short: a Param enters the graph either via a
Get
or via aRootRule
. Things that enter asGet
s do not need to be declared as roots. This is where the "root" in the name comes from: you only need to declare something aRootRule
if it might come in at the "root" of a graph: ie,scheduler.product_request
.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.
Oh, huh. We're declaring way too many
RootRules
then, I think, in part from bad advice I gave Benjy. I'll clean that up.cc @benjyw we shouldn't be using
RootRule
as much as I thought.