fix(utils): handle 0 as a reference value #325
Conversation
| } | ||
|
|
||
| return ref; |
There was a problem hiding this comment.
It looks like in the previous version a bad ref would result in this function returning "obj" (ref = obj on line 156, return ref here). Now it doesn't return anything if there's a bad ref. Don't know if this is good or bad, just wondering if it might have the potential to break something else.
There was a problem hiding this comment.
Oh good point I forgot to mention that part. So I don't know why we were returning the whole object, if you look at line 91 we were testing for truthiness, which if we passed an object back would always be true. Then in line 131 we set to_ret = ref which would be that object as well and return it. This is inside a string.replace function, and returning an object does nothing to the string. So this was always "working", but not how we thought it was...
There was a problem hiding this comment.
And hopefully all our glorious unit tests would catch anything major, but seems everything is in order.
There was a problem hiding this comment.
This file is so hard to understand - I'm concerned about the out-of-scope variable (updated_object) used within. I'm pretty sure I added the code that sets ref = obj as part of the work to provide better error output (although blame does not seem to think so), but I'm not clear how it interfaces with the updated_object variable.
There was a problem hiding this comment.
ok so updated_object is created immediate upon calling resolveObject. It is a deep clone of the object being resolved, to prevent any changes that are made during this process from trickling back outside the resolveObject scope. I think everything is ok as is...
|
LGTM! |
fixes amzn#324 Adding proper checking for alias/reference resolution so that 0 is a valid value. Now we check for undefined rather than "truthiness", because 0 is "false".
Issue #, if available: #324
Description of changes: Adding proper checking for alias/reference resolution so that
0is a valid value. Now we check for undefined rather than "truthiness", because 0 is "false".By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.