-
-
Notifications
You must be signed in to change notification settings - Fork 636
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 initial support for a parametrize
builtin to generate multiple copies of a target
#14408
Conversation
parametrize
builtin to generate multiple copies of a targetparametrize
builtin to generate multiple copies of a target
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.
Cool stuff :)
I've raised the question regarding the use of dict
rather than FrozenDict
and Mapping
for immutability. Maybe I've overlooked something.. ?
9ce6c3f
to
fc1a34f
Compare
fc1a34f
to
de0aee1
Compare
de0aee1
to
9cdd67e
Compare
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. Is. So. Awesome! :D
lgtm, but I haven't reviewed the changes to all backends.
f"{bullet_list(addr.spec for addr in generated)}\n\n" | ||
f"The address `{address}` was not generated by the target `{base_address}`, which " | ||
"only generated these addresses:\n\n" | ||
f"{bullet_list(addr.spec for addr in parametrizations.parametrizations)}\n\n" |
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.
Perhaps a moot point, but I tend to prefer relying on Address.__str__
for presentational messages. (which in this case is the same... currently.)
So the moot point is, that if that were to change for whatever reason in the future, it'll be much less impact if we didn't sprinkle with addr.spec
... ;)
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.
Yea, reasonable. Thanks!
|
||
target: Target | None | ||
generate_request = target_types_to_generate_requests.request_for(target_type) | ||
if generate_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.
This is a huge if block... possible/worth to split out (or perhaps await Joshuas rule parsing that supports following function calls)
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.
Sorry, I rushed this one in without doing this. I'm going to open a ticket for followup (since I'm out of town for the next few days), but will clean this up before 2.11.x
.
relative_file_path=self._relative_file_path, | ||
parameters=merged_parameters, | ||
) | ||
|
||
def maybe_convert_to_target_generator(self) -> Address: |
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.
Is target_generator
a misnomer? Maybe base_target
?
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.
Yea, it probably is. It used to be build_target
, although I like base
.
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.
Probably best as a followup though?
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.
Fine with me.
Maybe open an issue tracking all the little fixes like this? I know you have some dedicated tickets for bigger changes.
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.
Will do.
|
||
def build_file_aliases(): | ||
return BuildFileAliases( | ||
objects={"parametrize": Parametrize}, |
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.
V happy this is not a CAOF, I'm eagerly awaiting dropping that infrastructure.
…d Target templates. [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
9cdd67e
to
7280f3d
Compare
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.
It looks like this implementation lets you parametrize any field. As discussed on the design doc, I still strongly suspect that that is not optimal: there are some fields where it does not make semantic sense to parametrize, like skip_flake8
.
A major downside of parametrization if you're not careful is dependency inference will cause ambiguity. We have some blessed fields like resolve
where will update things to handle that, but it seems very wrong if we were to try to write something generically that has skip_flake8
in the key for module_mapping - you should be able to depend on something regardless of skip_flake8
.
Further, it's much easier to loosen the restriction than to add it.
I know you're trying to land this before heading out for a couple days, so this doesn't need to block landing this PR. But what do you think about this, and if we could in a followup add ParametrizableFieldMarker
?
|
||
for field_type in target.field_types: | ||
# TODO: Move to Target constructor. |
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.
Nack, we need to use the engine for singleton so that we don't warn hundreds of times.
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.
Yea... but we could use the @memoized
infrastructure for that, and it would cause it to be triggered in all cases, including for generated targets. Right now it's only triggered for generator targets, and only for non-moved fields.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
Thanks for making those changes!
(Approval is assuming that you're willing to entertain changing parametrization to not work with every field. I feel pretty strongly about this. But trying to not block.)
Yea, willing to entertain it. "dynamic" parametrization (which the design doc suggests will need a marker class) is still to come, so we'll be making more changes to resolution. My suspicion on needing to mark fields as supporting "static" parametrization though is that it is likely to add boilerplate to field definition without giving adequate benefit. If someone parametrizes the |
Boilerplate isn't compelling here imo. It's a single import from
True, but they might not make the connection why it's not a great idea to parameterize
Yeah, that is more compelling. This is where I think it's easier to loosen than restrict. If we find this is happening enough, we can open the floodgates. |
That depends whether the restriction is per-Field or per-Target. The latter case is the more likely one to be safe (how can I know that all usages of a
The warning would include |
As introduced in #13882 and designed in https://docs.google.com/document/d/1mzZWnXiE6OkgMH_Dm7WeA3ck9veusQmr_c6GWPb_tsQ/edit#, this change adds the
parametrize
builtin.Examples:
parametrize
may be used in any field (other thanname
) of non-generator targets, but is limited tomoved
fields of generator targets (i.e., those which will be moved from the generator to the generated target).Using
parametrize
in theoverrides
of a target generator requires adjustments to the target generator's implementation (see #14430): this change only adjusts the implementation for target file generators (TargetFileGenerator
).