Skip to content

Support of ./extensions/ directory in template#1340

Closed
con-f-use wants to merge 9 commits intocookiecutter:masterfrom
con-f-use:master
Closed

Support of ./extensions/ directory in template#1340
con-f-use wants to merge 9 commits intocookiecutter:masterfrom
con-f-use:master

Conversation

@con-f-use
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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!

@insspb insspb self-assigned this 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
Copy Markdown
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
Copy Markdown
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.

@con-f-use
Copy link
Copy Markdown
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
Copy Markdown
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

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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants