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

Support of ./extensions/ directory in template #1340

Closed
wants to merge 9 commits into from
Closed

Support of ./extensions/ directory in template #1340

wants to merge 9 commits into from

Conversation

con-f-use
Copy link

@con-f-use con-f-use commented Apr 10, 2020

In cookiecutter.json, you can now use the _extensions keyword to import extensions for a new ./extensions directory in the template (similar to ./hooks/).

image

For some related discussion see e.g.: #1211 and #547

The updated documentation has a more detailed description.

I would be willing to write additional tests to test this functionality.

Also. the "new in version ???" from my documentation should be updated before merging and releasing, same as release notes obviously.

@con-f-use
Copy link
Author

As mentioned, this is similar to #1240 and #944 .

@ssbarnea @insspb
I'm biased, but I'd gravitate toward my more simple solution that involves less code. For the same reason I'd also vote for using the _extensions keyword in the json for both local and installed extensions.

If anything, make the folder name in #1240 just extensions instead of local_extensions to go along with ./hooks/.

Copy link
Member

@insspb insspb left a comment

Choose a reason for hiding this comment

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

Tests!

cookiecutter/config.py Outdated Show resolved Hide resolved
docs/advanced/template_extensions.rst Outdated Show resolved Hide resolved
docs/advanced/template_extensions.rst Outdated Show resolved Hide resolved
@insspb insspb self-assigned this Apr 25, 2020
@insspb insspb added this to In progress in 2.0.0 Release via automation Apr 25, 2020
@insspb insspb added the feature This issue/PR relates to major feature request. label Apr 25, 2020
con-f-use and others added 3 commits April 25, 2020 17:00
@con-f-use con-f-use requested a review from insspb April 25, 2020 15:08
@insspb
Copy link
Member

insspb commented Apr 25, 2020

@con-f-use As I mention earlier some tests required.
At least these behavior cases should be covered:
User requested an custom extension, extension exist and work.
User requested an custom extension, but extension not exist in extension folder files.
User requested an custom extension, file/class exist in folder, but is nor an extension.

This feature can be useful for some part of users, who want to deliver own extensions without pip packages. But it is not just path extension.

Copy link
Member

@insspb insspb left a comment

Choose a reason for hiding this comment

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

Please look at comment above.

2.0.0 Release automation moved this from In progress to Pending approval Apr 25, 2020
@con-f-use
Copy link
Author

con-f-use commented Apr 25, 2020

User requested an custom extension, but extension not exist in extension folder files.
User requested an custom extension, file/class exist in folder, but is nor an extension

I wouldn't worry about these two cases. The python import system will catch the first one.

For the last, IMHO it is not necessary to check if it is an extension. If Jinja cannot find the extension, it will complain and I'd leave it to Jinja. Besides we have no way of knowing, whether the user wants to include a pip installed extension with "_extensions": "SomeName", or if SomeName is supposed to be in the ./extension directory but isn't.

I'd not explicitly forbid that something other than Jinja extension - it's actually a nice feature if it can be a normal python package or module in there and the directory is not permanently added to the path. Also, we already have arbitrary code execution by virtue of having hooks. Elaborate verification would just over-complicate things with no benefit.

That being said, I will write a test, that verifies an actual Jinja2 extension can be imported and used. Just not today ;-)

@insspb
Copy link
Member

insspb commented Apr 25, 2020

@con-f-use I just found #1240 and seems it is more correct implementation of same thing and almost done.
I will close this one and #944 as duplicates of #1240

@insspb insspb closed this Apr 25, 2020
2.0.0 Release automation moved this from Pending approval to Done Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate feature This issue/PR relates to major feature request.
Projects
2.0.0 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants