-
Notifications
You must be signed in to change notification settings - Fork 136
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
Triggers referring to hidden question will produce an error. #457
Triggers referring to hidden question will produce an error. #457
Conversation
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
==========================================
- Coverage 83.11% 83.11% -0.01%
==========================================
Files 25 25
Lines 3595 3600 +5
Branches 833 835 +2
==========================================
+ Hits 2988 2992 +4
Misses 460 460
- Partials 147 148 +1
Continue to review full report at Codecov.
|
Thanks Agus! I have not been able to break this. |
if nested_setvalues: | ||
for setvalue in nested_setvalues: | ||
msg = ( | ||
"Trigger was added for ${%s} that refers to hidden question. This is not allowed." |
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 found this error message hard to parse. How about flipping it around so it's something like "${%s} is not a user-visible question so it can't be used as a calculation trigger for question ${%s}"? The first string would be the trigger (self.name
) and the second would be the question that the trigger
is set for (setvalue[0]
). Hopefully I got that right.
I don't feel too strongly about it. If you prefer the current message, "to hidden question" -> "to a hidden question"
"Trigger was added for ${one-ts} that refers to hidden question. This is not allowed." | ||
], | ||
) | ||
|
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.
Were you thinking that a test for a typed calculate is not necessary because test_non_calculate_type_with_calculation_and_no_label_has_no_control
checks the branch in question.py
? I'd still add one for completeness (test_typed_calculate_cant_be_used_as_trigger
or something).
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'll merge and follow up with a PR for your consideration.
Closes #
#452
Why is this the best possible solution? Were any other approaches considered?
The simplest one.
What are the regression risks?
None.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
None.
Before submitting this PR, please make sure you have:
tests_v1
nosetests
and verified all tests passblack pyxform
to format code