introducing project mode #1022

rafalkrupinski · 2023-03-05T00:55:19Z

First commit extracts a Renderer class from Worker.
Not only Worker is huge and does a lot, but the new class greatly simplifies rendering the template for each project-mode item in the next commit.
The idea behind it was that it encapsulates the template and accepts answers to render the output.
I decided to leave _allow_render in Worker and pass it as a function argument, as it uses data stored in Worker. Let me know if you can think of a better way to split responsibility between those two.
Add looping over items

accept _items in copier.yaml as either a string or a list of strings. Internally it's always a list.
use each key to read a list of items from answers, and render its part of the template, stored under {{template_root}}/{{key}}
So if you have _items: ['a'], copier expects an answer a to be a list of values, and uses each value to render a sub-template stored in 'a'.
all directories under root ,that are named like the keys under _items are excluded from normal rendering.

Not sure how to structure the documentation. Needs changes to both the template and configuration parts. Shell I create a new file for the project mode?
If nobody minds, I'd like to disable the "identical" message for directories. I'm getting it a hundred of times when run in a loop and it doesn't do anything useful for directories.

example template: https://github.com/python-lapidary/lapidary-template/tree/test2

rafalkrupinski · 2023-03-05T08:36:44Z

For me these tests fail on master as well

sisp · 2023-03-05T11:45:11Z

They pass for me on master, and CI seems happy, too. 🤔

sisp

A couple of thoughts from my side:

If you think that the main.Worker class does too much and it would be better to factor out the rendering methods into renderer.Renderer, I suggest to do this refactoring in a dedicated PR to separate concerns and ease the review. In any case, @yajo will need to decide whether he likes the refactoring.
From a template creator's perspective, I find it a bit unexpected that the files in the folder marked via _items are merged in the target folder. For instance, in test_render_items_list() the files in both src/apples/ and src/oranges/ are generated to <dst>/ and not to <dst>/apples/ and <dst>/oranges/, respectively. I imagine there are technical reasons for this behavior, but the point of declarative project templates is to make the them readable and close to the generated project. Arguably, generating a directory structure is also in violation of this goal, but in that case the benefits outweigh the loss in readability IMO, and at least there is no implicit magic unlike the implicit merging of the src/apples/ and src/oranges/ folders.
I'm a bit confused by test_render_items_conditional(). The behavior tested there seems to equivalent to using a dynamic subdirectory. The only difference is the usage of the new special variable _item. Is the goal of this test case to test the rendering of dict items?
There was some minor discussion about nested loops in Making Copier generate a complete project #908. Would nested loops be possible with the current design? If yes, could you add a test case? I am vaguely worried that the current design might not be sufficiently flexible to address more advanced cases because the rendering context is only the main context plus the current item's data, but with nested loops the context from the parent loop seems to be unavailable in a child loop.

rafalkrupinski · 2023-03-05T18:07:40Z

A couple of thoughts from my side:

1. If you think that the `main.Worker` class does too much and it would be better to factor out the rendering methods into `renderer.Renderer`, I suggest to do this refactoring in a dedicated PR to separate concerns and ease the review. In any case, @yajo will need to decide whether he likes the refactoring.

Yeah, I thought it's easier to change from a single PR to multiple, than the other way around

2. From a template creator's perspective, I find it a bit unexpected that the files in the folder marked via `_items` are merged in the target folder. For instance, in `test_render_items_list()` the files in both `src/apples/` and `src/oranges/` are generated to `<dst>/` and not to `<dst>/apples/` and `<dst>/oranges/`, respectively. I imagine there are technical reasons for this behavior, but the point of declarative project templates is to make the them readable and close to the generated project. Arguably, [generating a directory structure](https://copier.readthedocs.io/en/latest/configuring/#generating-a-directory-structure) is also in violation of this goal, but in that case the benefits outweigh the loss in readability IMO, and at least there is no implicit magic unlike the implicit merging of the `src/apples/` and `src/oranges/` folders.

The first version I did was with only one items list and without the directory. In result, all files and directories under the template root had to be named with a condition ({% if _item %} or {% if not _item %}). I found that hard to read.
With multiple item lists it gets slightly worse.

Those directories are already treated specially - their contents is rendered once for every item. I would find it weird if they were copied over to the output, but I guess it's just a matter of expectations of a new user of Copier vs a seasoned one.

I'd rather not add it as a flag, bc removing the directories IMO would be more reasonable default, but not what older users might expect.

How about those directories go to _subtemplates directory to signify their special treatment?
e.g. src/_subtemplates/apples/* -> dst/*

3. I'm a bit confused by `test_render_items_conditional()`. The behavior tested there seems to equivalent to using a dynamic [subdirectory](https://copier.readthedocs.io/en/latest/configuring/#subdirectory). The only difference is the usage of the new special variable `_item`. Is the goal of this test case to test the rendering of dict items?

Good catch. It's from the previous version, updated a bit thoughtlessly.

4. There was some minor discussion about nested loops in #908. Would nested loops be possible with the current design? If yes, could you add a test case? I am vaguely worried that the current design might not be sufficiently flexible to address more advanced cases because the rendering context is only the main context plus the current item's data, but with nested loops the context from the parent loop seems to be unavailable in a child loop.

You're referring to this. I never intended to implement it due to exponential complexity of such templates.

Considering the goal of this change (generating SDK or class model from a machine readable description), I'd say nested loops are out of scope. If you're generating a class model, you have a single list of table definitions or JSON schemas to generate classes for - that's a single list. In case of SDK for OpenAPI, you have a bunch of schemas and operations, that's two lists, processed in parallel. All that assuming your model is large enough to render it as multiple files, otherwise you don't even need project mode.

Do you have a use case in mind?

rafalkrupinski · 2023-03-14T16:10:16Z

For me these tests fail on master as well

The tests mostly pass on an identical environment in my fork.
https://github.com/python-lapidary/copier/actions/runs/4333218244

rafalkrupinski · 2023-03-22T09:04:16Z

@yajo Any idea how to progress with this?

yajo

I find surprising also what @sisp said. I think that a looped template should behave as close as possible to a non-looped one.

My expectations are that it just gets rendered in loop under different context. Since the loop is sequential, any file that gets rendered more than once would get only the last result. It is up to the template designer to avoid that kind of situations; we just have to document that.

FWIW the normal thing would be that a file that gets rendered twice results in the same output if the context didn't change or _item isn't used within its content.

If it's needed to avoid that, then we can also add a key such as _dynamic_files with a list of gitignore-like patterns (just like _exclude) of files that should be rendered in loop. By default: all files. But that would be an extra thing to add later. I wouldn't include this in the MVP if possible.

So a simple design would be:

# src/copier.yaml
_items: "{{ classes }}"

classes:
  type: yaml
  help: write a list of classes you want to generate
  default: [one, two]

user: your name

Then another file named src/{{ _item }}.txt:

{{ user }}

I'd run copier -f src dst and expect to get:

dst/one.txt (contents: "your name")
dst/two.txt (contents: "your name")

Do you think you would be able to implement such design?

yajo · 2023-04-07T07:58:56Z

tests/test_project_mode.py

+            overwrite=True,
+        )
+
+        one_rendered = (dst / "one.txt").read_text()


Why this isn't like this? 🤔

Suggested change

one_rendered = (dst / "one.txt").read_text()

one_rendered = (dst / "items" / "one.txt").read_text()

yajo · 2023-04-07T07:59:46Z

tests/test_project_mode.py

+            / "copier.yml": """
+                _items: items
+            """,
+            src
+            / "items"


There are too many "items" things here! 😆

Please give each a different name to let me reason easily about what are you testing here. Something like this would be enough:

Suggested change

/ "copier.yml": """

_items: items

""",

src

/ "items"

/ "copier.yml": """

_items: items_var

""",

src

/ "items_dir"

yajo · 2023-04-07T08:01:20Z

copier/renderer.py

@@ -0,0 +1,174 @@
+from collections import ChainMap
+from dataclasses import dataclass, field


Probably you should change this to a pydantic dataclass, just to be consistent with the rest of Copier.

yajo · 2023-04-07T08:03:07Z

copier/renderer.py

+        return Renderer(
+            self.template,
+            self.subproject,
+            self._render_allowed,
+            self.pretend,
+            self.jinja_env,
+            self.answers_relpath,
+            ChainMap({"_item": items_value}, self._render_context),
+            items_key,
+            [],
+        )


You can do this probably:

Suggested change

return Renderer(

self.template,

self.subproject,

self._render_allowed,

self.pretend,

self.jinja_env,

self.answers_relpath,

ChainMap({"_item": items_value}, self._render_context),

items_key,

[],

)

return replace(

self,

_render_context=ChainMap({"_item": items_value}, self._render_context),

_items_key=items_key,

_items_keys=[],

)

yajo · 2023-04-07T08:03:56Z

copier/main.py

+        return cast(SandboxedEnvironment, self.abstract_jinja_env(SandboxedEnvironment))
+
+    @cached_property
+    def native_jinja_env(self) -> NativeEnvironment:


I don't see any uses of this...

yajo · 2023-04-07T08:05:02Z

copier/main.py

+            [raw_items]
+            if isinstance(raw_items, str)
+            else cast(collections.abc.Iterable, raw_items)


If it is a string, then it should probably get rendered using the native jinja env, to allow for complex scenarios. Right?

yajo · 2023-04-07T08:13:46Z

copier/main.py

+        return self._default_renderer.render_string(string)
+
+    @cached_property
+    def project_mode_keys(self) -> Iterable[str]:


All have been projects since forever... let's give it a better name:

Suggested change

def project_mode_keys(self) -> Iterable[str]:

def looped_mode_keys(self) -> Iterable[str]:

yajo · 2023-04-07T08:14:02Z

tests/test_project_mode.py

Call the file test_looped_mode.py

yajo · 2023-04-07T08:18:32Z

Oh and regarding the refactor... no problem with it.

sisp · 2023-04-07T10:02:36Z

@yajo Have you seen my comment in the other discussion about this topic? #908 (comment) I think that approach would cover a broader range of scenarios with more intuitive API. WDYT?

yajo · 2023-04-23T07:19:11Z

I like that design but i think it can be hard to implement. Gotta think about this deeply. El vie., 7 abr. 2023 11:02, Sigurd Spieckermann ***@***.***> escribió:

…

@yajo <https://github.com/yajo> Have you seen my comment in the other discussion about this topic? #908 (comment) <#908 (comment)> I think that approach would cover a broader range of scenarios with more intuitive API. WDYT? — Reply to this email directly, view it on GitHub <#1022 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHNXDPFB7OYJHTM6SSJRLDW77Q4PANCNFSM6AAAAAAVP3DKX4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

yajo · 2024-01-15T18:22:38Z

Closing because the feature has been designed in #1271 and this PR doesn't follow that design.

Thanks @rafalkrupinski for raising this subject! We'll wait until @0x00zer0day (or some faster volunteer) is able to publish the feature.

rafalkrupinski added 2 commits March 4, 2023 23:52

refactor(Worker): extract the rendering code to a separate class

42a031c

feat: project mode

066a76a

sisp reviewed Mar 5, 2023

View reviewed changes

yajo reviewed Apr 7, 2023

View reviewed changes

yajo closed this Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introducing project mode #1022

introducing project mode #1022

rafalkrupinski commented Mar 5, 2023

rafalkrupinski commented Mar 5, 2023

sisp commented Mar 5, 2023

sisp left a comment •

edited

Loading

rafalkrupinski commented Mar 5, 2023

rafalkrupinski commented Mar 14, 2023

rafalkrupinski commented Mar 22, 2023

yajo left a comment

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo Apr 7, 2023

yajo commented Apr 7, 2023

sisp commented Apr 7, 2023

yajo commented Apr 23, 2023 via email

yajo commented Jan 15, 2024

	one_rendered = (dst / "one.txt").read_text()
	one_rendered = (dst / "items" / "one.txt").read_text()

		@@ -0,0 +1,174 @@
		from collections import ChainMap
		from dataclasses import dataclass, field

	def project_mode_keys(self) -> Iterable[str]:
	def looped_mode_keys(self) -> Iterable[str]:

introducing project mode #1022

introducing project mode #1022

Conversation

rafalkrupinski commented Mar 5, 2023

rafalkrupinski commented Mar 5, 2023

sisp commented Mar 5, 2023

sisp left a comment • edited Loading

Choose a reason for hiding this comment

rafalkrupinski commented Mar 5, 2023

rafalkrupinski commented Mar 14, 2023

rafalkrupinski commented Mar 22, 2023

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Apr 7, 2023

sisp commented Apr 7, 2023

yajo commented Apr 23, 2023 via email

yajo commented Jan 15, 2024

sisp left a comment •

edited

Loading