-
-
Notifications
You must be signed in to change notification settings - Fork 197
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 support for computed values via skipped questions #1220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
=======================================
Coverage 96.74% 96.75%
=======================================
Files 47 47
Lines 3935 3942 +7
=======================================
+ Hits 3807 3814 +7
Misses 128 128
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Little improvement for docs. The rest looks great!
I'm thinking... We're just adding a feature, but what if a template relied on the fact that a skipped question was always missing? Should we label this as a breaking change? 🤔 |
Co-authored-by: Jairo Llopis <[email protected]>
Missing in the context? For this to be breaking, an injected variable through an extension like the context hooks one would need to have the same name as the skipped question. I guess, it can be breaking in rare cases. 🤔 I forgot to edit the inline comments to describe the new behavior. I'll push a commit with corrected comments in a moment. |
Well, the feature was never supported, so after all any breakages would come from unsupported use. Thus we can consider this a normal improvement. |
I think this PR isn't quite right yet. We should make sure that a skipped question's answer can only ever be its default value but not a value passed via |
Pull request was converted to draft
I think you don't need to care so deeply for this. Those are either weird or unsupported use cases anyways. If you do them, you know you're gonna get something wrong, so it should be OK for us IMHO. |
Alright, I guess we can keep it like it is now to ship it more quickly, possibly add more rigor later for those anyway weird cases. |
It seems you have style violations. Could you run pre-commit please? |
Ooops, yes, should be fixed now. |
Best PR 🥲 Many users will be super happy about this! |
thanks for your work on this @sisp! I can confirm that with this PR, my template at https://github.com/pydev-guide/pyrepo-copier works as it did before v8 |
More important is @yajo's careful assessment about the best implementation of this feature. |
I've added support for a long awaited feature: computed values via skipped questions (
when: false
). 🎉 See #1206 (comment) for more details.Supersedes #1206. Resolves #1204 #1205. Resolves (I guess ...) #1203. 😜