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 support for using LicenseRef- when initialising #697

Merged
merged 10 commits into from
Oct 24, 2023

Conversation

sgaist
Copy link
Contributor

@sgaist sgaist commented Mar 8, 2023

When initializing a new project that will use a custom license, the first step will fail because it currently handles only the list of supported license.

This pull request implements the handling for LicenseRef- so that it can also be used from the start.

The implementation will request a path to the license file so it can be copied over to the project.

Fixes #677

@mxmehl mxmehl force-pushed the 677_support_licenseref_init branch from a0b0f79 to 194b337 Compare April 6, 2023 13:19
@sgaist sgaist force-pushed the 677_support_licenseref_init branch from 194b337 to c083e8f Compare April 17, 2023 12:13
Copy link
Member

@nicorikken nicorikken left a comment

Choose a reason for hiding this comment

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

Thanks for the pull-request! It is great to accept other LicenseRef's this way.
I added a suggestion to make the regex more strict, which improves validation.
I think the input for a LicenseRef file can be improved by accepting a file instead of a directory too.
Tab completion on the file input would be nice, but I didn't find an easy way from the standard library to get that functionality.

src/reuse/download.py Outdated Show resolved Hide resolved
src/reuse/download.py Outdated Show resolved Hide resolved
src/reuse/init.py Show resolved Hide resolved
@sgaist sgaist force-pushed the 677_support_licenseref_init branch from 78f83e6 to e9843fd Compare April 27, 2023 13:52
@nicorikken
Copy link
Member

Wow, you're acting quickly. Just let me know when you think it is ready for another review.

@sgaist
Copy link
Contributor Author

sgaist commented Apr 27, 2023

You're catching me at a good time :-)

I think I have implemented or added all of your suggestions so you can go on for a new round.

@nicorikken
Copy link
Member

I gave it another test, much better now, thanks! I have two main remarks having tested it:

  • No warning or error if you don't provide anything.
  • No option to retry if a path if it failed to copy.

Maybe this is acceptable for now, as it mimics the behavior of other prompts.
Perhaps the error can be improved to hint to the user where to put it?
Could not copy LicenseRef-mylicense. Please add it manually as LicenseRef-mylicense.txt in the LICENCES/ directory.

Not sure about this. I'll ping @carmenbianca @mxmehl @linozen to hear what they think, before asking more work.

@mxmehl
Copy link
Member

mxmehl commented May 4, 2023

Thanks for the work @sgaist and the thorough review @nicorikken!

I gave it another test, much better now, thanks! I have two main remarks having tested it:

  • No warning or error if you don't provide anything.
  • No option to retry if a path if it failed to copy.

Maybe this is acceptable for now, as it mimics the behavior of other prompts. Perhaps the error can be improved to hint to the user where to put it? Could not copy LicenseRef-mylicense. Please add it manually as LicenseRef-mylicense.txt in the LICENCES/ directory.

I think it's acceptable. I would avoid to overengineer this as the number of users is probably quite small. How it's implemented now is a great improvement already, retaining the service init provides with custom licenses.

I'd appreciate Nico proposed error message to be added to this PR though as it directly provides users with concrete next steps when they see an error.

@sgaist
Copy link
Contributor Author

sgaist commented May 9, 2023

@mxmehl Sorry for the late reply, I missed your feedback.

I have simplified the code and added the error message as requested.

src/reuse/init.py Outdated Show resolved Hide resolved
Copy link
Member

@mxmehl mxmehl left a comment

Choose a reason for hiding this comment

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

Just one grammar mistake found, otherwise LGTM. Thanks!

@@ -31,6 +34,8 @@
"https://raw.githubusercontent.com/spdx/license-list-data/master/text/"
)

REF_RE = re.compile("LicenseRef-[a-zA-Z0-9-.]+$")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor issue, but should this be defined here or rather in _licenses.py to keep everything in the same central place?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion, but it might make sense to put it there. In the rest of the code base we don't use a regex for LicenseRef yet but just .startswith("LicenseRef-"), but it may well be that this will change for some reason someday.

Copy link
Member

Choose a reason for hiding this comment

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

@floriansnow can we merge this as is or do you want this changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicorikken I can live with both, but if we merge it as is, let's create an issue to clean this up a bit at a later date, together with the instances Max mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to create an issue, I moved the regexp and updated the LicenseRef- check occurrences.

@sgaist sgaist force-pushed the 677_support_licenseref_init branch from 10f780a to 096d9c3 Compare May 26, 2023 12:55
@sgaist sgaist requested a review from nicorikken May 26, 2023 12:59
@mxmehl mxmehl self-requested a review June 13, 2023 12:55
src/reuse/download.py Outdated Show resolved Hide resolved
@sgaist sgaist force-pushed the 677_support_licenseref_init branch from 096d9c3 to c845aa4 Compare July 12, 2023 11:23
sgaist and others added 4 commits October 24, 2023 14:35
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
SPDX dictates ASCII in the way they refer to the RFC5234 standard and repeat it through the specification.

Co-authored-by: Nico Rikken <nico@nicorikken.eu>
An empty path is now treated as a relative path which allows for classic
file not found error to be triggered.

An improved error message is provided.
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

Slightly refactored this and added some tests. Thanks for the PR @sgaist !

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
@carmenbianca carmenbianca merged commit 132762d into fsfe:main Oct 24, 2023
18 of 19 checks passed
@sgaist sgaist deleted the 677_support_licenseref_init branch October 24, 2023 17:16
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.

init does not accept LicenseRef
5 participants