Skip to content
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

feat: add dynamic file structures in loop using yield-tag #1855

Merged
merged 14 commits into from
Jan 18, 2025

Conversation

kj-9
Copy link
Contributor

@kj-9 kj-9 commented Nov 10, 2024

What I did:

  • Added a new Jinja extension in copier/jinja_ext.py to handle yield tags
    • This is a lightweight extension that detects yield tags and sets up the necessary context through jinja_env
  • Updated _render_path to return multiple destination paths and their corresponding contexts when yield tags are present
    • Internally, delegated the recursive logic for generating paths and contexts from yield tags to the _render_parts function
  • Added extra_context parameter to _render_file to enable injection of contexts generated from yield tags

kj-9 added 2 commits November 10, 2024 12:21
… extention for yield tag, allow _render_path to generate mulitple paths and contexts when yield tag is used
… test file to test dynamic file structure feature
@kj-9
Copy link
Contributor Author

kj-9 commented Nov 10, 2024

hopefully closes #1271

Copy link

codecov bot commented Nov 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (ca61418) to head (5f11c99).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1855      +/-   ##
==========================================
+ Coverage   97.77%   97.85%   +0.07%     
==========================================
  Files          49       51       +2     
  Lines        5261     5404     +143     
==========================================
+ Hits         5144     5288     +144     
+ Misses        117      116       -1     
Flag Coverage Δ
unittests 97.85% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sisp sisp self-requested a review November 12, 2024 20:56
@yajo
Copy link
Member

yajo commented Nov 20, 2024

I only reviewed tests in depth so far, and it seems the interface is almost exactly what I meant! Awesome!

Quoting from #1271 (comment):

are you rendering a filename?
No: just let the exception raise and print a nice message "Using yield outside of file names isn't supported".

That seems to be the only missing part. At least I saw no test for that. Could you add this please?

When done I'll review the implementation. This PR is big!

Thank you very much 😊

…d YieldTagInFileError and raise it if yield tag used when rendering file content. also add the test case
@kj-9
Copy link
Contributor Author

kj-9 commented Nov 22, 2024

@yajo

Thank you for your review!

I've added a new commit that implements the behavior you mentioned - throwing an exception when yield is used for rendering file contents. I've also added the corresponding test case.

Looking forward to your implementation review!

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Finally, the first code review is here!

Apart from what's mentioned, I also miss the documentation.

Thank you!

kj-9 added 4 commits December 1, 2024 13:10
…ntext with yield_name and yield_iterable attributes in YieldEnvironment and YieldExtension
…or check to _render_file method to enforce yield tag restrictions during file rendering
…r of parameters in _yield_support method for consistency and clarity
…ng over lists to generate files and directories
@kj-9
Copy link
Contributor Author

kj-9 commented Dec 2, 2024

@yajo
Thank you for the quick review!

I have added a few commits:

  • Fixes according to your comments
  • Minor code refactoring

And in the last commit, I added documentation. Specifically, three points:

  1. Added a row for the new feature in the comparison table (as far as I know, only copier supports this feature)
  2. Added jinja_ext.py to the Reference
  3. Added a section explaining the new feature to Configuring.

This is my first time adding documentation, so I'm not sure if it meets your expectations.
Could you please review it again?

@kj-9 kj-9 requested a review from yajo December 3, 2024 06:19
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Implementation looks very good, thanks!

Please review a couple of suggestions for the docs, so we can merge.

@copier-org/maintainers this is an important feature, would you like to add your reviews?

@sisp
Copy link
Member

sisp commented Dec 30, 2024

I'll try to give feedback later today.

Copy link
Contributor

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

A few suggestions, nothing critical, looking good 🙂

sisp
sisp previously requested changes Dec 30, 2024
Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this long-awaited feature, @kj-9! 🙏 I have a few conceptual remarks. WDYT, @kj-9 and @copier-org/maintainers?

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

This mostly LGTM, just a couple of minor comments. Thanks!

@yajo yajo dismissed sisp’s stale review January 18, 2025 08:17

Attended

@yajo yajo linked an issue Jan 18, 2025 that may be closed by this pull request
@yajo yajo enabled auto-merge (squash) January 18, 2025 08:19
@yajo yajo merged commit 557c0d6 into copier-org:master Jan 18, 2025
22 checks passed
@lbenka
Copy link

lbenka commented Feb 5, 2025

Hi, any idea when this is gonna be released in the main version, seems like it's only on master.

@pawamoy
Copy link
Contributor

pawamoy commented Feb 5, 2025

Copier doesn't have a schedule for releases, new releases are made when yajo finds the time to do so, and believes the time is right, mostly 🤷 A better question would be "Are there any blockers I could help with before a new release is published?" 🙏 😊

@yajo
Copy link
Member

yajo commented Feb 8, 2025

Sorry for the delay, I've been so busy lately...

I'll release when I have time, but you can install from master too in the mean time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate file structures on dynamic loop
5 participants