-
-
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: validate default values #1075
Conversation
50544e7
to
7d90d7d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1075 +/- ##
==========================================
- Coverage 96.59% 96.52% -0.08%
==========================================
Files 45 45
Lines 3699 3710 +11
==========================================
+ Hits 3573 3581 +8
- Misses 126 129 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@yajo I think this PR is ready for review now. There is one uncovered line, please see my inline comment.
copier/user_data.py
Outdated
try: | ||
type_fn = CAST_STR_TO_NATIVE[type_name] | ||
except KeyError: | ||
raise InvalidTypeError("Invalid answer type") |
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.
I'm not sure how to cover this error by a test as it doesn't occur in practice because the question type is checked to be one of the keys of CAST_STR_TO_NATIVE
before. Nevertheless, this function could theoretically be called in a different context, so I think it makes sense to keep the check here.
Could you please solve the conflicts? |
Done. |
I've added validation of default values which revealed an error in one of the test cases and some general weakness in the answer parsing/casting. In particular, a
type: str/int/float/bool
question could haveNone
as its default value and Copier didn't complain for several reasons:str
had a special treatment which seems obsolete to me after fix: require answer for questions without default value #958 has been merged.bool
silently handlesNone
wherebool(None) is False
.int
andfloat
both raise aTypeError
forint(None)
/float(None)
which got swallowed by a too broadtry
/except
block actually intended only for allowingtype: json/yaml
questions to parse answers with native (i.e. not stringified) Python values.Note: This PR requires #958 to be merged first. The test suite fails right now but passes for me after merging #958 locally.