-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
allow overriden defaults in questionnaire #477
Conversation
4fe6644
to
6ab06f3
Compare
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.
A little comment on code.
This feature is cool. With a bit of rework, it could fix #235.
Copier could support loading these custom defaults from $XDG_CONFIG_DIR/copier/user-defaults.yaml
. It could also feature a CLI flag --user-defaults
to allow the execution environment to override that path. Then, the custom defaults get loaded from there automatically.
Implementation would probably go around
Line 70 in 9f05fdf
class AnswersMap: |
Also, this still needs docs, tests, and a CHANGELOG entry to be merged.
Good work! Thanks 🙂
Codecov Report
@@ Coverage Diff @@
## master #477 +/- ##
==========================================
+ Coverage 95.65% 95.71% +0.05%
==========================================
Files 40 40
Lines 2580 2616 +36
==========================================
+ Hits 2468 2504 +36
Misses 112 112
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Cheers! To confirm - you believe this is an incremental step towards getting that functionality? I.E. it simplifies a subsequent MR (likely in cli.py?) that leverages the default-overrides available in the Worker class to merge these Walking that out a bit, we'll at some point want to merge (in order):
I think that logic could live entirely in cli.py - i.e. the caller would be responsible for reconciling these alternate default sources. |
ca13157
to
e1713a3
Compare
WRT to docs, the docstrings have been updated with the parameter. Please point me to where we'd want additional docs - it's one of these params that seems suited to the API reference, which is covered by the docstring. |
f0a32ae
to
53f25f3
Compare
In general terms, copier behaves the same through API and CLI. However it's true that this can be an exception because it's more ergonomic for an API call to provide user defaults through data than through a local file. So: yes, this is a valid intermediate step forward. |
8d3e905
to
d5981c6
Compare
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 friend! 🙂 The code looks nice, but it has some bugs. That also reveals the test is nice but not enough (as it should have detected those bugs). See code comments below.
c5606d9
to
72e7166
Compare
This works in conjunction with copier-org#476. If partial answers are written out to a file, they can be re-loaded as default overrides to whatever is specified in the template allowing one to resume progress.
72e7166
to
51b2e27
Compare
8b8ab3d
to
ef54292
Compare
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.
🎉
This works in conjunction with #476, but is independent of it. If partial answers are written out to a file, they can be re-loaded as default overrides to whatever is specified in the template allowing one to resume progress.
Some code that uses the combination of these two MRs:
Demo of these two features working together: https://asciinema.org/a/iVJvC8wAb8s3nSVKkmnhDjeZE