Closed tuxtina closed 8 years ago
This looks good to me. Thanks @tuxtina! Once the build passes, if someone else wants to review and hit the merge button that'd be great.
95.86%
Merging #667 into master will decrease coverage by -0.17% as of
7c211b8
@@ master #667 diff @@
======================================
Files 16 16
Stmts 1161 1161
Branches 0 0
Methods 0 0
======================================
- Hit 1115 1113 -2
Partial 0 0
- Missed 46 48 +2
Review entire Coverage Diff as of
7c211b8
+0.35%
via ...alidationMessages.js#25...28 +0.26%
via ...s/formlyUsability.js#18...20 +0.26%
via src/test.utils.js#38...40 Powered by Codecov. Updated on successful CI builds.
Good job investigating. I'm thinking, is it worth having explicit type checking here? The problem isn't really key === 0
, the problem is we aren't explicitly checking for undefined
@BarryThePenguin I assumed not checking explicitly for undefined
keys was intentional, as I see little use for falsy keys other than 0
. Do you see a use case for these? Or am I misunderstanding your comment?
Sorry, I'm on my phone so it's difficult to add detail :P
I think the reason this issue exists is because we were checking implicitly for falsy values. Instead, I think the check should be more explicit, maybe checking for typeof undefined and false.
I don't think that checking explicitly for 0 makes our intentions clear.
Hope that makes sense. Though I'm happy to see this merged this either way.
I would prefer to explicitly check for undefined
as well :+1:
For 0
, the rest of the code made it pretty clear that it was caught unintentionally by the check for !key
. @BarryThePenguin, you mention also checking for false
? What about null
, ''
, and NaN
? Which falsy keys should work?
Ignore false, that was a mistake. All that should be happening is an explicit check for undefined. In both cases the bellow should be suitable
if (angular.isUndefined(key) || angular.isUndefined(model)) {
return
}
The original issue was an implicit check for key
. What you have found and fixed is the fact that 0
was included in this implicit check. By explicitly checking for undefined
, the issue is resolved and the code clearly explains what is happening.
This is a separate issue though... Thank you for your contribution!
LGTM!
What
Change check in
parseSet()/parseGet()
to permit a numeric 0 key.Why
Looking at
parseSet()/parseGet()
, the intended behavior seems to be that numeric keys mean array indexes. However,parseSet()/parseGet()
will return prematurely if a0
key is used.How
Modified the check to permit
0
(and keep existing behavior for all other falsy values).For issue #658