-
-
Notifications
You must be signed in to change notification settings - Fork 235
Nested populated transclusions #2251
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
Conversation
…scluded files Also add the demo files for the test in `tests/demo_nested_transcludes`. When nested transcluded files are populated, the transclusion will result in a failure of the type: ```bash E yaml.composer.ComposerError: expected a single document in the stream E in "<byte string>", line 2, column 1: E !include ../copier_files/nested/ ... E ^ E but found another document E in "<byte string>", line 3, column 1: E --- E ^ ```
…g nested populated files This fix validates the test `tests/test_config::test_config_data_nested_transclusions`.
Changed from `demo_nested_trancludes` to `demo_transclude_nested`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for discovering this limitation of nested YAML includes and contributing support for nested multi-document YAML includes, @RR5555! 🙇
These are great discoveries and improvements! 👌 Just one request: The way I see it, this PR comprises 3 aspects, which I'd prefer to split into 3 PRs for better clarity and granularity:
-
Adding support for nested multi-document includes plus a test. This test happens to test also the base path of the includes, which is a different concern, so I'd limit this test to cover nested includes where all files are in the same directory.
-
Testing that nested include paths must all be relative to
copier.yml. As mentioned in 1., the test in this PR tests both the base path of the includes and nested multi-document includes. I'd create a dedicated PR with a test that asserts the include path to be relative tocopier.ymlwith only single-document includes. -
Documenting that nested include paths must all be relative to
copier.yml. I'd create a dedicated PR for this.Reading this added documentation in the context of the example shown in that subsection, I think the config directory and file names are more abstract. Could we make this a bit more concrete? Perhaps you have a concrete example that you could share that led to the discovery of the problem for which you're contributing a fix in this PR – although this documentation is only about the base path of the relative include paths? If not, feel free to let me know and I'll try to think of something.
4d0a12a to
063dd26
Compare
…the same directory Per review request. Refs: copier-org#2251#pullrequestreview-3076712643
|
Thank you very much for reviewing my PR ^^ DoneI have:
I will submit separate PRs for 2. & 3.. Please let me know if other modifications are required for this PR. Practical caseI indeed had a more practical case leading to the PRs: copier.yml
copier_files
|-project_details.yaml
|-Makefile
|-generated
|-generated_classifiers_include.yaml
|-project_framework.yaml
|-project_natural_language.yaml
|-project_topic.yaml
|-project_development_status.yaml
|-project_intended_audience.yaml
|-project_operating_system.yaml
|-project_typing.yaml
|-project_environment.yaml
|-project_license.yaml
|-project_programming_language.yaml flowchart LR
A[copier.yaml] --- Alink(includes) --> B[project_details.yaml] --- Blink(includes) --> C[generated_classifiers_include.yaml]
subgraph copier_files
B
Blink
subgraph generated
C --- Clink(includes)
Clink --> D[project_framework.yaml]
Clink --> E[project_natural_language.yaml]
Clink --> F[project_topic.yaml]
Clink --> G[project_development_status.yaml]
Clink --> H[project_intended_audience.yaml]
Clink --> I[project_operating_system.yaml]
Clink --> J[project_typing.yaml]
Clink --> K[project_environment.yaml]
Clink --> L[project_license.yaml]
Clink --> M[project_programming_language.yaml]
end
end
Hence, I encountered both the nested includes problem and the relative-to- |
Per review request. Note that if it is decided that pure path relativity is better, one just has to reverse the tests. Refs: copier-org#2251#pullrequestreview-3076712643
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2251 +/- ##
==========================================
+ Coverage 97.78% 98.00% +0.22%
==========================================
Files 55 55
Lines 6088 6111 +23
==========================================
+ Hits 5953 5989 +36
+ Misses 135 122 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sisp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉 Thanks, @RR5555! 🙏
|
Thank you for your reviews ^^ |
Issue
When trying to transclude nested populated (not just containing the include statement) files, we get the following:
This is due to the fact that
_includeinload_template_configis usingyaml.load, which does not seem to accept several "documents", that is namely the use of---, which to my understanding is necessary to both have questions and includes in the same yaml file.This PR
This PR introduces a test to show that failure case in the current state of the repo, along with a correcive patch.
It also adds a note how relative path for nested transclusions.
Test
The test function is
tests/test_config.py::test_config_data_nested_transclusions, and it uses demo files from directorytests/demo_transclude_nested.Patch
The patch basically:
Replaces:
By:
Which was already used in the first place to apply the first loading:
This change passes the new test, and works in practice.
Docs
Add a note that precise that nested transclusions have to be made relative to
copier.yml, not to each other.Possible future change or desiderata
Relative path all the way
The paths in the files are relative to
copier.ymland not to each other.It might be better if the file paths are relative to each other instead.
But, at least for now, this way works fine for what I wanted to personally do with my copier template.
Avoid circular transclusion
This is actually not a problem inherent to this patch, this problem is already present in the current master branch.
As these are not directly related to this patch, I will make a different PR to propose a test that results in:
It could be nice to catch a circular transclusion as soon as it happens instead of waiting for the recursion ceiling, and provides a more comprehensive message for the user such as which files collide.