Skip to content

Conversation

reneeb
Copy link
Contributor

@reneeb reneeb commented Jan 7, 2022

Currently the code checks for true-ness of the values set. But in Perl
the number 0 is false. It should be a valid value though. This commit
changes some checks so that defined-ness and length is checked. As 0 is
defined and has a length it is treated as a valid value.

This PR shouldn't be merged until the unit tests show that it is safe ;-)

…elds ;-)

Currently the code checks for _true_-ness of the values set. But in Perl
the number 0 is _false_. It should be a valid value though. This commit
changes some checks so that _defined_-ness and length is checked. As 0 is
_defined_ and has a length it is treated as a valid value.
@svenoe
Copy link
Contributor

svenoe commented Jan 10, 2022

I played around with it a bit. Actually I thought those routines would test the key of the DynamicField rather than its value, then I wouldn't have seen it as too problematic. I do not really understand why the values are checked, but for this especially you are right, of course. I'm a bit hesitant anyways, as in some cases for example the empty values are added as '' => '-',. Because seemingly the values itself are checked it is fine, but I'm not 100% certain if I have the full picture of every eventuality which could arise. Did you run the unit tests with this change being incorporated?

(I would say that even if this introduces some regressions, if they are not huge, we should rather fix those then, and change the logic of the ValueValidate as suggested by you. I just don't want to blindly merge...)

@reneeb
Copy link
Contributor Author

reneeb commented Jan 10, 2022

I'll run the unittests at the end of the week.

The empty value ('') is used to reset (delete the value) of the dynamic field. Adding manually the '' => '-' option should have a similar behaviour as the "add empty value" option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants