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

Add initial warnings regarding license-files glob patterns. #4838

Merged
merged 8 commits into from
Feb 28, 2025

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Feb 18, 2025

The PEP specifies that tools should reject any patterns which contain not-approved chars.
Furthermore, it should be an error if a pattern doesn't match any files.

Ref: #4829 (comment)

This by no means introduces an exhaustive check, just the low hanging fruits.

I intentionally do not analyse each pattern individually here to avoid wreaking too much havoc (imagine e.g. project templates that adds a very general catch all list of patterns in setup.cfg).

Regarding the glob patterns, I am keeping loose for the time being, would like to avoid adding a regex for each pattern if possible, but open to it...

/cc @cdce8p

Pull Request Checklist

@cdce8p
Copy link
Contributor

cdce8p commented Feb 18, 2025

I intentionally do not analyse each pattern individually here to avoid wreaking too much havoc (imagine e.g. project templates that adds a very general catch all list of patterns in setup.cfg).

I also started working on this before I saw you've opened this PR. As I chose a slightly different approach, I finished it so we could compare them better. See #4841

I explicitly decided to check that each glob pattern match at least one file as I think this is by far the most useful check to have. It's a fallback to make sure files actually get matched. I don't think project templates are a big deal. They either didn't add it in the first place as setuptools provided good defaults early on or used a value specific to their configuration. Why should they have added multiple patterns if they didn't use these files?

@abravalheri
Copy link
Contributor Author

Thank you very much @cdce8p, I tried to incorporate the validations from #4841 into this one according to the discussion in #4841 (comment).

Copy link
Contributor

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Looks good to me. Adding the enforce_match argument makes a lot of sense. Will go ahead and close my PR then.

@abravalheri
Copy link
Contributor Author

abravalheri commented Feb 21, 2025

@abravalheri
Copy link
Contributor Author

Interestingly after I merged #4842 and rebased the PR, pytest decided to start ignoring the decorator @pytest.mark.filterwarnings("ignore"):

image

@cdce8p
Copy link
Contributor

cdce8p commented Feb 21, 2025

Waiting feedback from https://discuss.python.org/t/pep-639-round-3-improving-license-clarity-with-better-package-metadata/53020/174.

Not sure if you'll even get a response. Just checked hatch and pdm-backend as these were the first to add PEP 639 support. However, both only seem to do at best a fairly limited validation.

Hatchling only seems to check that the pattern is a str and uses glob afterwards.
https://github.com/pypa/hatch/blob/64031c1cf5d02d85203f68cb7a4fd5db2aa7a004/backend/src/hatchling/metadata/core.py#L758-L768

Pdm-backend doesn't even do glob matching for user supplied values 🤷🏻‍♂️
https://github.com/pdm-project/pdm-backend/blob/4bd3755565a37667457a7a7d079be5f347a551c4/src/pdm/backend/base.py#L277-L291

Poetry hasn't added support yet AFAIK and flit isn't a good comparison since I contributed the pattern validation there.

--
To summarize, the checks as they are right now, seem to match the specification. Maybe that's good enough for now. If someone reports an issue / deviation from the spec, it's always possible to refine it later.

@cdce8p
Copy link
Contributor

cdce8p commented Feb 21, 2025

Interestingly after I merged #4842 and rebased the PR, pytest decided to start ignoring the decorator @pytest.mark.filterwarnings("ignore")

In #4841 I used monkeypatch. That would probably work here as well by patching _find_pattern.

@abravalheri
Copy link
Contributor Author

Than you very much @cdce8p. I updated the PR based on your feedback.

Copy link
Contributor

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

LGTM. As mentioned earlier, I'm not sure there will be feedback on this in the forum. It's probably a case for iterative improvements. See what works and where the pain points are.

@abravalheri abravalheri merged commit 5bb11cb into pypa:feature/pep639 Feb 28, 2025
21 of 24 checks passed
@abravalheri abravalheri deleted the license-files branch February 28, 2025 10:31
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.

2 participants