Some additions

[jpiron]

I can split this into several PRs if you prefer

[KostyaEsmukov]

Thanks for the interest in the project!

The “Makefile typo” commit and “YAMLConfig “ one mostly look good to me, but regarding the new strategies I have some questions, so I guess we should either settle that first or you could move the two good commits to a separate PR to get them merged quicker.

(Also let’s wait for @kxepal opinion as well)

[KostyaEsmukov]

Assuming that usually cookiecutter retrieves context with an interactive input, where only strings can be entered, I think the values should be kept as strings.

I understand that in yaml any data type can be used, but relying on non-string values in templates might make it impossible to enter them in interactive rollup.

[jpiron]

As cookiecutter supports dict values one can expect that Scaraplate does too.

I updated the YAMLConf docstring to :

{%- for key, value in cookiecutter.items()|sort %}
{%- if value is mapping %}
{{ key }}: {{ value }}
{%- else %}
{{ key }}: "{{ value }}"
{%- endif %}
{%- endfor %}

and set the read method return type to Dict[str, Union[Dict, str]]
to support dict values while preserving.
Unfortunately I can't set this Dict[str, Union[Dict, str]] return type on the CookieCutterContext.read method because of this.

[KostyaEsmukov]

As cookiecutter supports dict values one can expect that Scaraplate does too.

Interesting, I didn’t know that. Do you have any idea how these dicts can be provided when entering values interactively via cli?

and set the read method return type to Dict[str, Union[Dict, str]]
to support dict values while preserving.

I guess this type should also allow lists (and, in fact, be recursive, which is not supported by mypy yet). If we choose to allow dicts, I think it would be okay to stick with Dict[str, Any]. But the bigger problem would be adding support for dicts to the already existing configparser-based implementations, which don’t provide support for arbitrary data types.

[KostyaEsmukov]

Could you please provide any usecase for this strategy? Preferably in the docstring.

config is supposed to be the “context” of a strategy. Having both config and extra_context might further complicate things. Also I would like to let the context be filled via cli even on a first rollup, and then the assumption that an empty extra_context means a first rollup would become broken.

So I would prefer to find an alternative solution (which we might try come up on together after discussing the usecases).

[jpiron]

Two main uses cases here:

The first is that our cookiecutters initialize the git repository of the target project if there is no .git folder in it (this is done as part of our post_gen_project hook).
As Scaraplate renders the project in a temporary folder, the git repository is always created. And when it tries to write the genrated .git folder to the actual target folder it fails because of the git files not being the same as the ones already present.

The second use case is that our cookiecutters generate some sample files (both for code and for tests) that are very likely to be renamed or deleted. We'd like them to be generated on the first rollup only to ease further rollups.

Apart from these use cases, providing strategies with the "previous context" might allow some more advanced strategies.

I agree that relying on the presence of this extra_context to infer rollup status is weak and ends broken with the feature you mentioned.
A solution could be append scaraplate metadata to generated context. Something like :

[cookiecutter_context]
...
[scaraplate_context]
rollups_count = 1
git_project_url = git_remote.project_url()
commit_hash = commit_hash
commit_url = git_remote.commit_url(commit_hash)
is_git_dirty = _is_git_dirty(template_path)

What do you think of this ?

[KostyaEsmukov]

These usecases are actually good, thank you. I agree that the already existing strategies cannot cover them.

Apart from these use cases, providing strategies with the "previous context" might allow some more advanced strategies.

Or might make strategies’ implementations more complex. I would prefer to not resort to this until there’s no other way.

I agree that relying on the presence of this extra_context to infer rollup status is weak and ends broken with the feature you mentioned.
A solution could be append scaraplate metadata to generated context. Something like :

[cookiecutter_context]
...
[scaraplate_context]
rollups_count = 1
git_project_url = git_remote.project_url()
commit_hash = commit_hash
commit_url = git_remote.commit_url(commit_hash)
is_git_dirty = _is_git_dirty(template_path)

What do you think of this ?

I’m afraid it will create unnecessary noise when rolling up templates which don’t change anything in the target projects except this section with context.

If I’m not missing something out, currently the initial rollup can be reliably detected by performing a boolean check on the CookieCutterContext.read output. So it seems to me that adding a new boolean arg to the Strategy’s __init__ like is_initial_rollup should be a good compromise between complexity and functionality.

[jpiron]

Great if this can be done without modifying the strategy signature ! I am in holidays for the next 2 weeks but I will definitely look at this once I return.