Skip to content

Add license parameter#701

Merged
cjolowicz merged 17 commits intocjolowicz:mainfrom
paw-lu:license-parameter
Mar 8, 2021
Merged

Add license parameter#701
cjolowicz merged 17 commits intocjolowicz:mainfrom
paw-lu:license-parameter

Conversation

@paw-lu
Copy link
Copy Markdown
Contributor

@paw-lu paw-lu commented Dec 15, 2020

This PR adds a license parameter. Currently there are three options: MIT, Apache-2.0, and GPL-3.0. All references to MIT and MIT License throughout the template are replaced with cookiecutter.license and cookiecutter.full_license_name, respectively.

Additionally two new parameters are added to the template—year and full_license_name. I had originally meant for these two variables to be private rendered variables that begin with __ so the user does not interact with them, but it seems that this feature is not yet released.

The logic for full_license_name is honestly a bit ugly.

The TimeExtension is one of the template extensions included with Cookiecutter, so it will not require the user to install any additional dependencies.

Closes #4

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Dec 15, 2020

GitHub previews of the added license files are hosted as a gist.

The Apache-2.0 text was written by me and the GPL-3.0 was taken directly from gnu.org.

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Dec 15, 2020

Nox's pre-commit run is causing is causing the checks to fail. The issue seems to be with prettier, but only on Python 3.9. Prettier had passed locally.

Curiously, the output diff doesn't make sense to me 😅

nox > pre-commit run --all-files --show-diff-on-failure
black....................................................................Passed
Check for added large files..............................................Passed
Check Toml...............................................................Passed
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
flake8...................................................................Passed
Reorder python imports...................................................Passed
Trim Trailing Whitespace.................................................Passed
prettier.................................................................Failed
- hook id: prettier
- files were modified by this hook

.cookiecutter.json

pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/.cookiecutter.json b/.cookiecutter.json
index f0d8565..1379d39 100644
--- a/.cookiecutter.json
+++ b/.cookiecutter.json
@@ -1,7 +1,5 @@
 {
-  "_extensions": [
-    "jinja2_time.TimeExtension"
-  ],
+  "_extensions": ["jinja2_time.TimeExtension"],
   "_template": "cookiecutter-hypermodern-python",
   "author": "Claudio Jolowicz",
   "email": "mail@claudiojolowicz.com",

Because if we look at that line , it does not match the removed portion of the diff, but does match the reported "change".

Will investigate later!

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Dec 15, 2020

Realized I was mistaken!

Prettier is complaining about the generated .cookiecutter.json in the project itself, as opposed to the template's cookiecutter.json.

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Dec 16, 2020

While the documentation suggests otherwise, it seems you don't need to specify the included extensions.

In fact I noticed we were using the cookiecutter.extensions.JsonifyExtension extension in .cookiecutter.json without specifying it in the _extensions variable.

I'm not sure how fragile this is as it's undocumented (will you need to specify extensions in the future?).

The latest commit removes the _extensions variable—causing all tests to pass, though this issue might resurface.

Another option would be to add a .prettierignore.

.cookiecutter.json

Am not sure why the 3.6–3.8 tests passed though 🤔

@paw-lu paw-lu marked this pull request as ready for review February 9, 2021 16:41
Copy link
Copy Markdown
Owner

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, this looks nice!

I think we can get rid of the year variable, and inline the tag where needed. See how pydanny/cookiecutter-django do this, for example.

In fact, I would somewhat prefer to have only license as a Cookiecutter variable, and replace occurrences of {{ cookiecutter.full_license_name }} with {{ cookiecutter.license.replace("-", " ") }} license, even if it does not look quite as nice in the rendered output.

Your solution avoids having a hook moving or removing license files, and I like that. Recently I noticed there is a third way to deal with this problem, and I thought I'd mention it here: You could have separate license files with conditionals in their names, like this for the MIT license: {% if cookiecutter.license == 'MIT' %}LICENSE.rst{% endif %}. Files with an empty name will be ignored by Cookiecutter. Not sure though if that solution is actually better than what we have here, and quite happy to leave this as it is.

Base automatically changed from master to main February 18, 2021 19:02
@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Feb 19, 2021

Ended up aggreeing with all of your suggestions, and implimented them.

I had originally written up this PR under the assumption that cookiecutter private variables were available—so variables such as year and full_license_name were not exposed to the user—but it seems this is for the yet to be released 2.0.0 veersion of cookiecutter. When that feture is launched it might be worth implimenting them again as private variables.

You could have separate license files with conditionals in their names, like this for the MIT license: {% if cookiecutter.license == 'MIT' %}LICENSE.rst{% endif %}.

Did not know this! Thanks for letting me know. I went ahead and implimented this. I think both have their (big) cons. Everything in one file—like I originally had—makes it difficult to parse the Jinja logic. On the other end, having multiple files can lead to a cluttered and confusing project root. For now we only have three LICENSE.rst variants—so I think that's okay. I have no problem reverting to one file if things get too ugly.

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Feb 19, 2021

I see that the Windows test is failing now due to an invalid path because of the Jinja-y file naming 😅.

@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Feb 19, 2021

" is an invalid character for file names in Windows. Changing to '.

Copy link
Copy Markdown
Owner

@cjolowicz cjolowicz 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 update! I left some small suggestions inline, and some more thoughts below.

This PR will require some changes in the User Guide, specifically under Creating a project (the list of project variables), and Documentation (the description of LICENSE.rst). Possibly also the section Files and directories, for the documentation files, depending on how you handle the GPL (see below).

One thing I noticed is that for GPL-3.0 and Apache-2.0, the license text does not contain an actual copyright notice, only advice for how to add it to the project. I took a quick look at how pydanny/cookiecutter-django and ionelmc/cookiecutter-pylibrary handle this. The Django template places the copyright notice in LICENSE and includes the full text as the traditional COPYING file; but it only does this for the GPL. Ionel's template precedes the full license text by the copyright notice and a horizontal separator. Personally, I like Ionel's approach because it's pragmatic without losing clarity. What do you think?

@paw-lu paw-lu force-pushed the license-parameter branch from f1da315 to de1c987 Compare March 8, 2021 04:24
@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Mar 8, 2021

Thanks for the suggestions! I got a little sloppy there with the replacement 😅. Thanks for catching that awkard wording. I don't think those result in valid reStructuredText links though—so I modified them a bit.

This PR will require some changes in the User Guide, specifically under Creating a project (the list of project variables), and Documentation (the description of LICENSE.rst).

Good catch!

One thing I noticed is that for GPL-3.0 and Apache-2.0, the license text does not contain an actual copyright notice, only advice for how to add it to the project.

Good eye. I actually based my implimentation based on what GitHub does by default if you select GPL-3.0 or Apache-2.0 as the license for the repo, so it's interesting that GitHub itself doesn't actually apply the notice. I placed the notice right before the full text on both files.

@paw-lu paw-lu force-pushed the license-parameter branch 2 times, most recently from ac3bd6c to 8595be6 Compare March 8, 2021 04:30
@cjolowicz
Copy link
Copy Markdown
Owner

LGTM 💯 Can you rebase on main? That should take care of the CI failure.

@paw-lu paw-lu force-pushed the license-parameter branch from 8595be6 to d3201a3 Compare March 8, 2021 20:55
@paw-lu
Copy link
Copy Markdown
Contributor Author

paw-lu commented Mar 8, 2021

Rebased!

Copy link
Copy Markdown
Owner

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Thank you!

@cjolowicz cjolowicz merged commit 7bb6d01 into cjolowicz:main Mar 8, 2021
@cjolowicz cjolowicz added the enhancement New feature or request label Mar 8, 2021
@cjolowicz cjolowicz mentioned this pull request Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add license parameter

2 participants