fix: modifying start and end variable strings by sacha-c · Pull Request #1963 · cookiecutter/cookiecutter

sacha-c · 2023-10-18T12:30:04Z

Description

Cookiecutter supports using _jinja2_env_vars to modify the jinja environment. However when modifying variable_start_string or variable_end_string it will not work because we are hard-coding the start and end strings for the main directory creation (see here). In addition, the jinja context is not always populated with the _jinja2_env_vars, for example when getting the placeholder prompts here

Technical details

This change does 3 things (which are in separate commits for readability):

Creates a common utility function create_env_with_context so the jinja environment is created similarly and with context each time.
- Note that I also considered creating a single jinja env object & passing it around, however it requires much more refactoring (especially of tests), and we need to create it at least twice anyway (once to create the cookiecutter prompt, and once after the prompts are passed to the context), so I decided to go for the easier solution for now.
Leverages the _jinja2_env_vars to search for the initial cookiecutter directory, to allow this customization of variable_start_string and variable_end_string.
Small refactor to move prompt_and_delete from utils to prompt. It was causing circular import issues when having prompt import utils, because utils was importing prompt. And in any case I would say it makes more sense for it to live in prompt because it is a prompt as well after all.

I also removed the ensure_dir_is_templated check because the same logic is used to find the dir, and is therefore redundant.

Use-case
In react-native projects, it's extremely common to use {{ and }} to pass objects to components. For example:

<MyComponent
    some-prop={{ key: 'value' }}
</MyComponent>

Since {{ is the default jinja variable string prefix, jinja goes crazy with react-native templates 🤪 .
A solution to this is to set _jinja2_env_vars.variable_start_string == '{%{' (or something else). But cookiecutter currently has issues when overriding this setting.

Testing

Tested locally on a react-native project with _jinja2_env_vars.variable_start_string == '{%{' and _jinja2_env_vars.variable_end_string == '}%}'

sacha-c · 2023-10-24T14:24:39Z

(EDIT: This comment is now resolved with my latest change)

After further testing, I found that the placeholders used to generate the context are rendered without using the _jinja2_env_vars.

This means that currently a file with:

{
    "name": "Easy Project",
    "slug": "{%{ cookiecutter.name.lower().replace(' ', '-') }%}",
    "repo_path": "elements/elements-react-native/{%{ cookiecutter.slug }%}",
    "author_name": "John Doe",
    "author_email": "john.doe@elements.nl",
    "description": "The greatest project ever created.",
    "use_renovate": [
        "yes",
        "no"
    ],
    "_jinja2_env_vars": {
        "variable_start_string": "{%{",
        "variable_end_string": "}%}"
    }
}

will not work, because cookiecutter renders {%{ cookiecutter.name.lower().replace(' ', '-') }%} using {{ instead of the provided override {%{

The issue is that prompt_for_config is being called without passing the envvars to the jinja context...

I am setting this PR to draft and will try to make the improvements for this to work.

EDIT: I have updated the PR with a solution. Looking forward to your feedback 🙂

Cookiecutter supports using _jinja2_env_vars to modify the jinja environment. However when modifying variable_start_string or variable_end_string it will not work because we are hard-coding the start and end strings for the main directory creation. This change leverages the _jinja2_env_vars to search for the main directory, to allow this customization of variable_start_string and variable_end_string. I also remove the `ensure_dir_is_templated` check because the same logic is used to find the dir, which is therefore redundant.

prompt_and_delete was a function in utils which also made use of read_user_yes_no which was in prompt. This means that prompt could not import utils because it would create a circular import. This change moves prompt_and_delete to prompt, so that prompt may make use of util functions

kurtmckee · 2023-11-15T02:33:52Z

@sacha-c Thank you for your work on this! However, this appears to be a duplicate of #1837.

I'm closing this as a duplicate, but please comment back if there's functionality that your PR addresses that isn't addressed in #1837. Thank you!

sacha-c · 2023-11-15T08:13:58Z

@kurtmckee thanks for taking a look! I really appreciate it 👍

And thanks for linking that PR, I had totally missed it when investigating this issue.

However reading through it it looks like it is missing an additional fix
Basically there are two issues:

variable_start_string & variable_end_string are not taken into account when looking for the cookiecutter root directory
- This is included in the PR you linked, and in mine
_jinja2_env_vars not taken into account when generating the updated cookiecutter context
- This is missing from the PR you linked, I only discovered it while further testing my changes on a project. see my comment above

I did like the use of Environment.variable_start_string rather than hardcoding default values as I was doing, so I've updated my PR with that improvement

I would suggest closing the other PR in favor of this one.

kurtmckee · 2023-11-15T11:15:21Z

Oh, nice! Thanks for responding back!

...and I just discovered that I only have repo permissions to close PRs but not to re-open them. I assumed that I could re-open PRs. 😞

@ericof and @jensens, please re-open this PR and review in light of the additional bug fix that this offers over #1837 (interpreting template openers/closers in variables in cookiecutter.json).

sacha-c · 2023-11-29T10:20:47Z

Hello!
I see this PR is still closed, so just want to make sure it is still on the radar, if it can be re-opened and reviewed
Thanks! @kurtmckee @ericof @jensens

sacha-c · 2023-12-13T08:12:30Z

Hello,
What is the state of this PR, would you like me to create a new PR in order reopen it?
Thanks in advance
@kurtmckee @ericof @jensens

kurtmckee · 2023-12-13T12:22:29Z

I cannot re-open PRs; @ericof it will be helpful if you can reopen this. Thanks!

kurtmckee · 2023-12-13T12:23:13Z

@sacha-c If this doesn't get re-opened, please open a new PR. My apologies for these difficulties!

ericof · 2023-12-13T21:43:44Z

Unfortunately I could not reopen (at least not in mobile app)

sacha-c · 2023-12-16T14:31:54Z

I have just recreated the PR as requested: #1997
I've rebased it onto current main & all tests are green, so all should be good and ready for review
@kurtmckee @ericof @jensens

sacha-c changed the title ~~fix: modifying start and env variable strings~~ fix: modifying start and end variable strings Oct 18, 2023

sacha-c force-pushed the fix/jinja-variable-directory-name branch from bb4f688 to 84e7447 Compare October 18, 2023 12:32

sacha-c mentioned this pull request Oct 18, 2023

Cannot change jinja2 delimiters because NonTemplatedInputDirException exception #1736

Closed

sacha-c force-pushed the fix/jinja-variable-directory-name branch from 84e7447 to 09a106c Compare October 23, 2023 07:56

sacha-c mentioned this pull request Oct 23, 2023

Cannot change jinja2 variable_start_string and variable_end_string, NonTemplatedInputDirException #1966

Closed

sacha-c marked this pull request as draft October 24, 2023 14:19

sacha-c added 3 commits November 6, 2023 17:13

fix: create jinja environment using context

9474074

sacha-c force-pushed the fix/jinja-variable-directory-name branch from 09a106c to 97f9126 Compare November 7, 2023 11:01

sacha-c marked this pull request as ready for review November 7, 2023 11:10

kurtmckee closed this Nov 15, 2023

sacha-c mentioned this pull request Dec 16, 2023

fix: modifying start and end variable strings #1997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: modifying start and end variable strings#1963

fix: modifying start and end variable strings#1963
sacha-c wants to merge 3 commits intocookiecutter:mainfrom
sacha-c:fix/jinja-variable-directory-name

sacha-c commented Oct 18, 2023 •

edited

Loading

Uh oh!

sacha-c commented Oct 24, 2023 •

edited

Loading

Uh oh!

kurtmckee commented Nov 15, 2023

Uh oh!

sacha-c commented Nov 15, 2023

Uh oh!

kurtmckee commented Nov 15, 2023

Uh oh!

sacha-c commented Nov 29, 2023 •

edited

Loading

Uh oh!

sacha-c commented Dec 13, 2023

Uh oh!

kurtmckee commented Dec 13, 2023

Uh oh!

kurtmckee commented Dec 13, 2023

Uh oh!

ericof commented Dec 13, 2023

Uh oh!

sacha-c commented Dec 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sacha-c commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sacha-c commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtmckee commented Nov 15, 2023

Uh oh!

sacha-c commented Nov 15, 2023

Uh oh!

kurtmckee commented Nov 15, 2023

Uh oh!

sacha-c commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sacha-c commented Dec 13, 2023

Uh oh!

kurtmckee commented Dec 13, 2023

Uh oh!

kurtmckee commented Dec 13, 2023

Uh oh!

ericof commented Dec 13, 2023

Uh oh!

sacha-c commented Dec 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sacha-c commented Oct 18, 2023 •

edited

Loading

sacha-c commented Oct 24, 2023 •

edited

Loading

sacha-c commented Nov 29, 2023 •

edited

Loading