Skip to content

Render variables starting with an underscore.#1339

Merged
ssbarnea merged 15 commits intocookiecutter:masterfrom
smoothml:feature/render-skipped-variables
May 27, 2020
Merged

Render variables starting with an underscore.#1339
ssbarnea merged 15 commits intocookiecutter:masterfrom
smoothml:feature/render-skipped-variables

Conversation

@smoothml
Copy link
Copy Markdown
Contributor

@smoothml smoothml commented Apr 8, 2020

Currently, variables starting with an underscore (which are not shown to the user on project creation) are not rendered with Jinja2. If you want to create one of these variables based on other variables you cannot. For example, if you are creating a Python package repository and want to base the package name on the project name you might have a configuration like this

{
    "project_name": "Project Name",
    "project_slug": "{{ cookiecutter.project_name|lower|replace(' ', '-') }}",
    "package_name": "{{ cookiecutter.project_name|lower|replace(' ', '_') }}",
}

Which could create a structure like this

project-name
├── Makefile
├── README.md
├── requirements.txt
└── src
    ├── project_name
    │   └── __init__.py
    ├── setup.py
    └── tests
        └── __init__.py

However, the user can change project_slug and package_slug. This may be desirable, but (as in my current situation) it may not be. You could create post-generation hooks to fail project creation if these are changed, but this seems inelegant. This PR allows for the configuration to be specified like this

{
    "project_name": "Project Name",
    "_project_slug": "{{ cookiecutter.project_name|lower|replace(' ', '-') }}",
    "_package_name": "{{ cookiecutter.project_name|lower|replace(' ', '_') }}",
}

The user then cannot change these variables, enforcing consistent naming.

@smoothml smoothml marked this pull request as ready for review April 8, 2020 17:43
@con-f-use
Copy link
Copy Markdown

con-f-use commented Apr 10, 2020

I like this very much, as I ran into that very problem as a relatively new user. Perhaps you should add a test and documentation for that as well. Btw. have you checked how that interacts with the replay function / repo updater packages?

I'd very much like to see this PR merged soon and making it easy on the developers is a good way.

@smoothml
Copy link
Copy Markdown
Contributor Author

@con-f-use I'm glad you like it! You're right, I should add a test and some docs, though as far as I could see the underscore variables aren't explicitely tested for or documented (I didn't spend too long looking so may have missed it).

have you checked how that interacts with the replay function / repo updater packages?

I haven't, but all the tests pass so I assume this is fine.

I'm not sure this will be merged given #1256 Perhaps this could go in the 2.0 release @insspb?

@insspb insspb added enhancement This issue/PR relates to a feature request. needs tests PR Only: This PR require additional tests needs-docs PR Only: This PR require additional documentation labels Apr 14, 2020
@insspb
Copy link
Copy Markdown
Member

insspb commented Apr 14, 2020

Hi @smoothml
First of all - thank you for contribution. I really like this idea. Why I do not done it myself? Haha :)
Then here is my thoughts about it.

  1. Remaking how current underscore works can break current templates. Maybe somebody read this code before and used it as feature for example with numbers. In your case numbers will be converted to string.
    Here is code for you, to reproduce what I am talking about:
    def test_should_render_dict(self):
        """

        """
        context = {
            'cookiecutter': {
                'project_name': 'Slartibartfast',
                '_project_name': 1123,
                'details': {'other_name': '{{cookiecutter.project_name}}'},
            }
        }

        cookiecutter_dict = prompt.prompt_for_config(context, no_input=True)
        assert cookiecutter_dict == {
            'project_name': 'Slartibartfast',
            '_project_name': 1123,
            'details': {'other_name': u'Slartibartfast',},
        }

This code will be good with current implementation. Will fail with your proposal.

So...

Let do double underscore for user rendered private variables + docs + test. And it can be included in nearest release after this will be done.

But I really like this idea. At least things like this will be shorter: https://github.com/ionelmc/cookiecutter-pylibrary/blob/master/cookiecutter.json

Currently included in 1.7.1, but as we are plan to make a release in few days maybe it will not be ready for that time. At this case i will move it to 2.0.

@smoothml
Copy link
Copy Markdown
Contributor Author

Thanks for your comments @insspb. Good point regarding numbers vs. strings. Double underscores work 👍 I'm not sure I'll be able to get this ready in the next few days so getting it in 2.0 is a good aim I think.

@ssbarnea ssbarnea requested a review from insspb April 15, 2020 07:37
@ssbarnea ssbarnea added this to the 1.8.0 milestone Apr 15, 2020
@ssbarnea
Copy link
Copy Markdown
Member

I like this change but please add some tests for it and we should merge it in next major release 1.8.0, not the patch one (1.7.x) due to the risk of breaking existing templates.

The risk of breaking templates is small and template authors should already have CI/CD configured to test their templates with cookiecutter from master, running jobs at least weekly. We will always wait at least one week before merging a breaking change and tagging a release or pre-release.

@ssbarnea ssbarnea self-requested a review April 15, 2020 07:42
cookiecutter_dict = prompt.prompt_for_config(context)
assert cookiecutter_dict == {'_copy_without_render': ['*.html']}

def test_render_hidden_variables(self):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docstrings missed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, thanks. I haven't finished yet!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I should have made it a draft PR actually. I'll do that.

@smoothml smoothml marked this pull request as draft April 15, 2020 17:47
@insspb
Copy link
Copy Markdown
Member

insspb commented Apr 25, 2020

@smoothml still draft?

@smoothml
Copy link
Copy Markdown
Contributor Author

Yeah, sorry. Been slammed the last couple of weeks. Just docs to go. Will try and get it done this weekend. Sorry for the delay!

@smoothml smoothml marked this pull request as ready for review April 28, 2020 19:34
@smoothml smoothml requested a review from insspb April 28, 2020 19:34
@smoothml
Copy link
Copy Markdown
Contributor Author

@insspb @ssbarnea apologies for the delay. This is ready for review now.

@insspb insspb modified the milestones: 1.8.0, 2.0.0 May 26, 2020
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. needs tests PR Only: This PR require additional tests needs-docs PR Only: This PR require additional documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants