Adding support for dynamic defaults. #385
Conversation
Codecov Report
@@ Coverage Diff @@
## master #385 +/- ##
=========================================
+ Coverage 82.42% 82.5% +0.08%
=========================================
Files 23 23
Lines 3260 3281 +21
Branches 763 770 +7
=========================================
+ Hits 2687 2707 +20
Misses 433 433
- Partials 140 141 +1
Continue to review full report at Codecov.
|
| if not self.default or not self.dynamic_default(): | ||
| return | ||
|
|
||
| if self.parent.__class__.__name__ == "Survey" and in_binding: |
There was a problem hiding this comment.
If anyone have comment on how to better do this, reply to this please.
|
The tests look good. For someone who wants to think about this at a high level, the proposed approach is to treat a default as dynamic if it contains one or more balanced pair of brackets ([]), parens (()) or curly braces ({}). The only odd case I can think of that this would miss is an expression on numeric literals like I also am not certain that it's so important for brackets/parens/braces to be balanced. I'd tend to treat anything that contains any of |
|
@lognaturel you are thinking of all of the arithmetic ops there, right? Adding arithmetic ops will make it think |
|
@lognaturel and the second question, does code-cov update the coverage or just checking it once? |
There was a problem hiding this comment.
I've made some comments inline about the strategies for identifying dynamic defaults. I think @MartijnR should comment on this because Enketo users will be affected by overly aggressive identification of dynamic defaults until Enketo adds support for them. @tiritea may also have thoughts.
ODK Collect users on older versions will also be affected in that the form will fail to open. This is not great because the error they get will be cryptic. However, the form would not have worked as indented when converted with prior versions of pyxform, either. For this reason, I don't believe dynamic defaults are very common. The likeliest source of problem will be someone who designs a form with dynamic defaults, tries it on a version of Collect v1.24+, and then pushes it out to users who have older Collect versions. This is painful but hopefully not fatal because folks generally do a test run when working with new forms.
- @nribeka add a warning if any dynamic default is identified with text "This form definition contains dynamic defaults. Not all form filling software and versions support dynamic defaults so you should test the form with the software version you plan to use."
- @MartijnR review the strategy for identifying dynamic defaults
pyxform/survey_element.py
Outdated
| expression = [] | ||
| contains_dynamic = False | ||
| arithmetic_construct = {"*", "/", "+", "-"} | ||
| if self.type == "date": |
There was a problem hiding this comment.
If all clients supported dynamic defaults, I would say that this case should be removed because it could be useful to have math in a date default. Given that not all clients support them, this does seem like the safest thing to do.
|
@lognaturel added the warning. I moved the checking for dynamic to utils instead. |
It's not necessary and doesn't work as expected. See discussion at XLSForm#385 (comment)
Fix #338.
This will add dynamic default handler for the form. This failed validation on both enketo and ODKValidator I think.