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

Mark field values from __defaults__ as being a default value. #20950

Closed
wants to merge 6 commits into from

Conversation

kaos
Copy link
Member

@kaos kaos commented May 23, 2024

Part 2 of 2 fixes #20947. (Part 1: #20949)

When providing default values using __defaults__ for Python source globs, they are now treated as a default value, rather than a explicit value. The implication is on how pants treats multiple globs for the two cases. For defaults, as long as any glob matches an existing file, all is well, where as for explicit globs, all of them must match existing files.

This was problematic when overriding the default source globs for the python_sources target for instance to make it recursive, as the defaults list many globs, some of which may not be applicable.

Now, the experience augmenting the default globs are seamless (no more unmatched globs issues):

# Make `python_sources` targets recursive by default.
__defaults__({
  python_sources: dict(
    sources = [
      "**/" + glob for glob in
      python_sources.sources.default   
    ]
  ),
  ...
})

@kaos kaos added the category:bugfix Bug fixes for released features label May 23, 2024
@kaos kaos requested a review from a team May 23, 2024 13:19
@kaos
Copy link
Member Author

kaos commented May 23, 2024

Only 78fdb9f that needs reviewing in this PR. The rest comes from #20949.

@huonw
Copy link
Contributor

huonw commented May 25, 2024

Thanks for working on this.

Does this cause these values to be hidden from pants peek --exclude-defaults? Maybe that's desirable, but potentially is a behaviour change we should call out? Either that or change to have two levels of defaults (built-in vs user-supplied)?

@kaos
Copy link
Member Author

kaos commented May 25, 2024

Thanks for working on this.

Does this cause these values to be hidden from pants peek --exclude-defaults? Maybe that's desirable, but potentially is a behaviour change we should call out? Either that or change to have two levels of defaults (built-in vs user-supplied)?

Good call, I didn't think of this, but yea, as-is it will be excluded here! (nice! 😎) but I also think it's a good idea to be able to distinguish builtin defaults from user provided defaults. Perhaps as a new option that defines what peek considers being default for exclusion: builtin_defaults, user_defined_defaults or any_defaults.

@kaos
Copy link
Member Author

kaos commented May 27, 2024

As discussed on #20947 we opt to change the behavior for source globs instead of having a conditional on the value being default or not.

@kaos kaos closed this May 27, 2024
@kaos kaos deleted the kaos-defaults-as-builtin branch May 27, 2024 07:37
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.

Values from __defaults__ are not treated the same as builtin field defaults.
2 participants