Skip to content

Core: Fix __defaults__ to also apply for generated file targets.#20649

Merged
kaos merged 3 commits intomainfrom
kaos/20595-defaults-generated-targets
Mar 18, 2024
Merged

Core: Fix __defaults__ to also apply for generated file targets.#20649
kaos merged 3 commits intomainfrom
kaos/20595-defaults-generated-targets

Conversation

@kaos
Copy link
Copy Markdown
Member

@kaos kaos commented Mar 7, 2024

When providing defaults for a target generator, such as python_sources, the defaults now apply to the generated python_source targets.

Closes #20595

Plugin API note:

This works for all TargetGenerator classes that declares the generated target type using the generated_target_cls class var.

# The generated Target class.
generated_target_cls: ClassVar[type[Target]]

@kaos kaos added the category:bugfix Bug fixes for released features label Mar 7, 2024
assert target_adaptor.kwargs["tags"] == ["24"]


def test_generated_target_defaults(target_adaptor_rule_runner: RuleRunner) -> None:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is implemented in graph.py, but the __defaults__ feature, as used in the BUILD files, are otherwise tested here so I felt this was the logical place for this test but could be convinced to move it to graph_test.py.

Comment on lines +266 to +270
# Pre-load field values from defaults for the target type being generated.
if hasattr(target_type, "generated_target_cls"):
family = await Get(AddressFamily, AddressFamilyDir(address.spec_path))
template_fields = dict(family.defaults.get(target_type.generated_target_cls.alias, {}))
else:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the only new in this function. It had to move before the @rule method calling it.

Comment on lines -438 to -439
# Split out the `propagated_fields` before construction.
template_fields = {}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This part is the changed bit in the moved function, the rest is unchanged.

Copy link
Copy Markdown
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

LGTM!

And also serves as a reminder that our target mechanisms are much too complicated... :)

@kaos kaos merged commit e294001 into main Mar 18, 2024
@kaos kaos deleted the kaos/20595-defaults-generated-targets branch March 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:bugfix Bug fixes for released features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

__defaults__ should apply also for generated targets

2 participants