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

Feature/local extensions #1240

Merged
merged 11 commits into from Mar 18, 2022

Conversation

mwesterhof
Copy link
Contributor

@mwesterhof mwesterhof commented Oct 9, 2019

this is a proposed change to add a "_local_extensions" feature.
It will enable the use of an additional "_local_extensions" element in the cookiecutter.json file. This will have the exact same functionality as the "_extensions" element, except that it can be used to load jinja2 template extensions straight from the project template.

I'm not certain if this is the best approach to achieve this, but I'm happy to have some feedback, of course.

{
    "directory_name": "directory_name",
    "module_name": "module_name",
    "_local_extensions": ["local_extensions.SomeExtension"]
}

@insspb insspb added 2.0.0 enhancement This issue/PR relates to a feature request. labels Dec 22, 2019
@insspb insspb added this to In progress in 2.0.0 Release via automation Dec 22, 2019
@croesnick
Copy link
Contributor

croesnick commented Jan 20, 2020

I totally missed your PR, @mwesterhof, when quickly scanning the open PRs today for exactly this feature. While not being finally decided whether I like an implicit (silent) loading of local extensions or your more explicit approach via dedicated keyword, I definitely like your context-manager approach for prepending repo_dir to sys.path. 🙂

@mwesterhof
Copy link
Contributor Author

mwesterhof commented Jan 21, 2020

@croesnick

I totally missed your PR, @mwesterhof, when quickly scanning the open PRs today for exactly this feature. While not being finally decided whether I like an implicit (silent) loading of local extensions or your more explicit approach via dedicated keyword, I definitely like your context-manager approach for prepending repo_dir to sys.path.

Thanks, it was a bit of a challenge to find the right way to implement this. I'd be very interested in this feature, though. When you decide on the way you want this to work, feel free to let me know. I'd be happy to make some changes

@insspb insspb self-assigned this Feb 1, 2020
@insspb
Copy link
Member

insspb commented Feb 1, 2020

@mwesterhof @croesnick thank you for contribution.
As you both are active, we can speed up this feature implementation.
First it should be well documented from point how user should it use.

@mwesterhof mwesterhof force-pushed the feature/local_extensions branch from d8bb40f to e950ef9 Compare Feb 25, 2020
@mwesterhof
Copy link
Contributor Author

mwesterhof commented Feb 25, 2020

@insspb @croesnick
i added some docs for this feature to the "advanced" section. I haven't specified a version number yet, of course... should I leave that the way it is, or can i just replace the X.x with the next upcoming version?

@ssbarnea ssbarnea added this to the 2.0.0 milestone Mar 30, 2020
@ssbarnea ssbarnea removed the 2.0.0 label Mar 30, 2020
@con-f-use
Copy link

con-f-use commented Apr 12, 2020

I'm sorry, it seems I've implemented a very similar thing in #1340 but at least mine is more documented than the other duplicate in #944 😅

For completeness I want to mention the issues, I know of, that have requested something like that #1211 and #547 so that people don't re-implement it again.

Btw. may I ask, why not just add the extension directory to the path. It will only stay there, as long as the interpreter process running cookiecutter lives, so it will not influence anything else. You don't even have to check of its existence as python ignores non-existent dirs on sys.path.

I'd gravitate toward the 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 package installed extensions.

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

@grahamalama
Copy link

grahamalama commented May 10, 2020

Love this PR @mwesterhof! I was thinking about how to tackle the modify config from a hook problem that others were having, and basically came to the same idea that you did here.

I had one idea that I thought might improve this implementation. I haven't fully thought it through, but I wanted to get your take:

As a cookiecutter user, I don't want to have to know / care about how to write a custom Jinja filter -- I just want to write a python function (or functions) that I can use in my cookiecutter.json.

What about instead of having users create a full-blown extension like:

class FoobarExtension(Extension):
    def __init__(self, environment):
        super(FoobarExtension, self).__init__(environment)
         
        def foobar():
             return "foobar"
        
        environment.filters['foobar'] = foobar

we had them create a module filters.py where they'd define a bunch of functions:

def foo():
   return "bar"

def baz():
   return "qux"

and we could do something like:

import inspect

import filters

functions = inspect.getmembers(filters, inspect.isfunction)
for function in functions:
    register_function_as_filter(function)

Thoughts on this?

@mwesterhof
Copy link
Contributor Author

mwesterhof commented May 13, 2020

it's an interesting take, @grahamalama
I'm generally in favor of that kind of simplification, but i have a feeling it might be a bit too restrictive and too "magical".

I'm no expert on jinja, but i think we're ignoring a lot of the power of template filters by simplifying it this much. I'd propose a compromise:

@simple_filter
def foo():
    return 'stuff'
  1. creating fully fledged jinja-based filters is still possible
  2. creating a simple function based one is possible
  3. registering the function based filters is explicit
  4. it's possible to add other (helper) functions to the module without them erroneously being picked up as filters

it actually mimics some ideas in the django framework regarding template filters and template tags, and it's an approach that i'm a big fan of.

Any opinions? I'd be happy to implement this behavior in my branch

@con-f-use
Copy link

con-f-use commented May 13, 2020

I still maintain, that just adding the extensions directory to the path and letting the user do whatever is the best, smallest, most simple and most generic solution.

I don't know what the extra _local_extensions keyword (the existing _extensions keyword works if the extensions dir is on the path) and elaborate import_patch... functionality brings to the table other than unnecessary complexity.

Either way, the simple_filter decorator could be imported from cookiecutter.utils or some place in the cookie-cutter namespace, though, as long as it's well documented. I like it.

@mwesterhof
Copy link
Contributor Author

mwesterhof commented May 13, 2020

@con-f-use i would agree. importing the decorator as an additional helper from cookiecutter.utils is exactly what i had in mind. As far as separating the local extensions from the "regular" ones goes... i'm sure this is somewhat of a personal thing, but I'm personally a fan of having a clear distinction between "extensions bundled with the template" and "installed extensions". That's just my view on it, but i'd be happy to change that. I'm not entirely sure who makes that call, but please let me know

@mwesterhof
Copy link
Contributor Author

mwesterhof commented Jun 12, 2020

@insspb @croesnick @con-f-use @grahamalama
i just resolved a few conflicts, and I'm wondering if anyone has time to look at this MR. I'm more than happy to implement some changes, if necessary

@gasbasd
Copy link

gasbasd commented Jun 27, 2020

any news about this?

@@ -414,6 +414,19 @@ def test_echo_unknown_extension_error(tmpdir, cli_runner):
assert 'Unable to load extension: ' in result.output


def test_local_extension(tmpdir, cli_runner):

Choose a reason for hiding this comment

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

Would it be good to include tests for non-happy path? I can think of two cases, but maybe there are more:

  • I include a filter in my template, but didn't include the extension in the _local_extensions list
  • I include a filter in my template and extension in the _local_extensions list, but haven't defined that extension (or it's somehow not available to be imported)
    • It looks like there's a similar test for built-in Jinja extensions

Copy link
Contributor Author

@mwesterhof mwesterhof Jul 3, 2020

Choose a reason for hiding this comment

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

that's a good idea. i'll pick that up ASAP

This example assumes that a ``local_extensions`` folder (python module) exists in the template root.
It will contain a ``main.py`` file, containing the following (for instance):

Choose a reason for hiding this comment

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

If I only need one or two extensions and they're not that complicated, I can just define them directly in a module local_extensions.py, correct? Maybe instead of having the documentation suggest a more-complicated package setup, it could suggest simply that you have a module local_extensions available in the template root with extensions defined, with no reference to a folder or main.py. That would leave it up to the user to use a standalone module or a package structure depending on their needs.

Copy link
Contributor Author

@mwesterhof mwesterhof Jul 3, 2020

Choose a reason for hiding this comment

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

that seems fair. i'll make the change

Choose a reason for hiding this comment

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

From what I gather reading the code you are adding the path of the local directory. So in effect you could create a folder that is named extensions that contains an __init__.py file. In that file you'll have the extensions and you'd refer to that in the cookiecutter.json file as 'extensions.MyExtension' and it should all work.

@jonathan-s
Copy link

jonathan-s commented Jul 15, 2020

I was looking for exactly this. I'm glad that this in the works. My preference, would be to not introduce new naming such as the _local_extensions and instead use the already existing _extensions. From reading the code on how you are patching the python path that shouldn't be a problem.

@jonathan-s
Copy link

jonathan-s commented Jul 15, 2020

@mwesterhof

it actually mimics some ideas in the django framework regarding template filters and template tags, and it's an approach that i'm a big fan of.

Any opinions? I'd be happy to implement this behavior in my branch

I like the @simplefilter approach as well.

@mwesterhof
Copy link
Contributor Author

mwesterhof commented Jul 15, 2020

sorry guys, i've been quite sick recently and haven't gotten around to any work. I'll start working on this again though, so expect to see changes soon 💪

2.0.0 Release automation moved this from Done to In progress May 12, 2021
@simobasso simobasso modified the milestones: 2.0.0, Next May 21, 2021
@simobasso simobasso removed this from In progress in 2.0.0 Release May 21, 2021
@jonathan-s
Copy link

jonathan-s commented Jul 5, 2021

Can we please get this in the 2.0 release?

@matrixise
Copy link

matrixise commented Mar 3, 2022

Hello, is there any update for this feature? Thank you

@con-f-use
Copy link

con-f-use commented Mar 3, 2022

@audreyfeldroy Pretty please? 😁

@frankhuurman
Copy link

frankhuurman commented Mar 11, 2022

2 days ago I've opened up an issue requesting to use some sort of custom .py script or function while generating the cookiecutter project to set a value in the cookiecutter.json, I believe this feature is exactly what I'm looking for! (In my case I need to read out the git config file to grab the name of the user)

Is there any update on this?

This is the one feature needed to automate all the things with cookiecutter 😛

@con-f-use
Copy link

con-f-use commented Mar 11, 2022

Yeah, please merge this asap, I'd prefer not to wait another 2.5 years. It would be incredibly useful!

@matrixise
Copy link

matrixise commented Mar 14, 2022

Hi, I hope the best for @insspb but unfortunately, there is no Github activities for his account since December 2021.

Could we review/merge this PR without him?

Thank you,

@mwesterhof
Copy link
Contributor Author

mwesterhof commented Mar 15, 2022

@insspb indeed doesn't seem available long term. I'm sorry to once again have to bother @ssbarnea , but is there any way to proceed with this PR without his input?

@ssbarnea ssbarnea closed this Mar 15, 2022
@ssbarnea ssbarnea reopened this Mar 15, 2022
@ssbarnea
Copy link
Member

ssbarnea commented Mar 15, 2022

I was planning to merge it but I wanted to see the CI result first.

@mwesterhof
Copy link
Contributor Author

mwesterhof commented Mar 15, 2022

@ssbarnea it seems that black linting was added somewhere along the line. I accounted for that, and it looks like everything passes now

@ssbarnea ssbarnea merged commit c0e7698 into cookiecutter:master Mar 18, 2022
17 checks passed
@mwesterhof mwesterhof deleted the feature/local_extensions branch Mar 18, 2022
@con-f-use
Copy link

con-f-use commented Mar 18, 2022

I'm overjoyed 😁 Thanks ssabernea! 🥇 When can we expect that to be released?

Edit:
Included in 2.1.0 released on May 30th, 2022.

@frankhuurman
Copy link

frankhuurman commented Mar 22, 2022

Awesome, can't wait to use it cause this will really make things a lot more extensive!

Same question as @con-f-use, is there a planned release date(with updated Docs as well?).

@connerxyz
Copy link

connerxyz commented Mar 23, 2022

I'm eager make use of this feature too! Thanks.

}

This example assumes that a ``local_extensions`` folder (python module) exists in the template root.
It will contain a ``main.py`` file, containing the following (for instance):
Copy link
Contributor

@alkatar21 alkatar21 Apr 20, 2022

Choose a reason for hiding this comment

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

main.py must be __init__.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue/PR relates to a feature request. feature This issue/PR relates to major feature request. high-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet