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

fix: accept other filenames for LICENSE #327

Merged
merged 7 commits into from
Nov 30, 2023
Merged

fix: accept other filenames for LICENSE #327

merged 7 commits into from
Nov 30, 2023

Conversation

jfrost-mo
Copy link
Contributor

@jfrost-mo jfrost-mo commented Nov 30, 2023

The added filenames are LICENCE and COPYING, both of which are commonly used. There might be others accepted by GitHub, but the list can easily be extended.

This check could be a little slower due to having to do three times the number of string comparisons, but hopefully that is counteracted by the switch to using a generator, ensuring that an intermediate list doesn't have to be built, and that the check can terminate as soon as a file is found.

Fixes #326

The added filenames are LICENCE and COPYING, both of which are
commonly used. There might be others accepted by GitHub, but the
list can easily be extended.

This check could be a little slower due to having to do three times
the number of string comparisons, but hoefully that is counteracted
by the switch to using a generator, ensuring that an intermediate
list doesn't have to be built, and that the check can terminate as
soon as a file is found.

Fixes #326
@henryiii
Copy link
Collaborator

The full list from the draft PEP 639 and what I've used elsewhere is "LICEN[CS]E*", "COPYING*", "NOTICE*", "AUTHORS*". I think that might make sense. Though we want the check to pass if you have a license file, and AUTHORS* alone is not supposed to be a license file, so that one probably shouldn't be included. Actually, NOTICE also is a supplement. So yes, I think the list you provided is ideal, but what do you thing about making it "startswith" instead of "in"?

jfrost-mo and others added 2 commits November 30, 2023 17:18
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@jfrost-mo
Copy link
Contributor Author

jfrost-mo commented Nov 30, 2023

what do you thing about making it "startswith" instead of "in"?

As p.name is a string, in checks if the spelling is a substring. So it would be true for "LICENCE", "COPYING.md", as well as "GPL-LICENSE.txt".

It is also currently case sensitive, as that is the common convention.

@henryiii
Copy link
Collaborator

henryiii commented Nov 30, 2023

But "GPL-LICENSE.txt" would not be a valid license name according to PEP 639, it would need to be "LICENSE-GPL.txt". That auto-search is implemented in several places, like hatchling, scikit-build-core, setuptools, pdm. Also poetry and flit, though those are one LICENSE spelling only, does cover COPYING* too. All of these expect the name at the start of the file.

@jfrost-mo
Copy link
Contributor Author

Fair enough. I'll swap to checking it is at the start.

@henryiii
Copy link
Collaborator

Okay, sounds good, I'll trigger the CI once you are done.

@henryiii
Copy link
Collaborator

Actually, I think pre-commit did it for me. :)

@jfrost-mo
Copy link
Contributor Author

Should all be good now.

@henryiii henryiii merged commit fbdc192 into scientific-python:main Nov 30, 2023
15 checks passed
@henryiii
Copy link
Collaborator

Thanks!

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.

Should recognise alternate spellings of LICENSE
2 participants