Fix Required Issue with Heterogenous Arrays #546
Conversation
sophypal
changed the title
| } else if ('properties' in bunsenModel) { | ||
| bunsenModel = bunsenModel.properties[pathSegment] | ||
| } else { | ||
| return undefined |
There was a problem hiding this comment.
This is a condition doesn't seem to exist in the original code. Do we have a use case in bunsen that we would want this condition to be hit?
There was a problem hiding this comment.
I hit it. That's why I added it. If the path doesn't match the model, then this can happen.
type: 'object'
properties: {
a: {type: 'string'}
}
Works when the path is a. Doesn't work when the model is a.b. The caller is looking for undefined to end the traversal loop and handling the condition here makes sure we don't end up in a situation where it tries to index into an undefined object.
addon/utils.js
Outdated
| @@ -390,6 +382,22 @@ export function getErrorMessage (error) { | |||
| } | |||
| /* eslint-enable complexity */ | |||
|
|
|||
| export function getSubModel (pathSegment, bunsenModel) { | |||
There was a problem hiding this comment.
Can we have some simple JSdoc for the future code reader?
There was a problem hiding this comment.
Does JSDoc really help though? I purposely deleted some JSDoc from bunsen-core because I realized that not once did I look at them nor did I find them descriptive enough to warrant their usage.
There was a problem hiding this comment.
So I can add it if you find it useful. It maybe be just me that don't find it useful and find it easier to follow code. Just note that in the near future, I plan to refactor all so we can centralize all the different flavors of model traversals.
There was a problem hiding this comment.
Actually, I was just about to ask the question why did you remove some JSDoc in bunsen-core. I think they still provide some value to developers like me that are not very familiar with the code base and make a complex project less scared.
There was a problem hiding this comment.
Perhaps. I thought this in the past. I do value JSDoc for documenting types still but most often I find this kind of JSDoc,
Gets the sub model
@param {Object} input - the input object
getSubModel (input)
How is that useful? Also when getSubModel starts getting more complex, the JSDoc is often left out of maintenance. So you find yourself reading the past instead of the present. There was a study that was done that found that comments lead to more bugs. This conclusion was that developers would read through the comments instead of relying on reading what the code did.
There was a problem hiding this comment.
So I'm going to put some JSDoc in here if only to document the types properly. I still think the comments for this small function will serve little value (not I didn't say no value). The larger confusion new developers will have is not necessarily understanding what this function explicit does but how it functions in the larger scheme of things.
There was a problem hiding this comment.
That's true. When I read through the comments/docs, I did assume that they're correct and up to date. In this case, I agree in this case it's not proving too much value except types.
There was a problem hiding this comment.
There's a specific JSDoc that I think it shouldn't be removed in bunsen-core.
https://github.com/ciena-blueplanet/bunsen-core/pull/135/files#diff-30699e11bc5c44658541e1afec32d6f5L3
This one does provide some conceptual idea of what does this function do.
There was a problem hiding this comment.
That whole section is hard to read honestly. I'd have to agree that comments did make it more approachable but if I was the one reviewing it I'd actually force the developer to make the code cleaner. Not all comments are bad but I'd rather document that kind of thing in an architecture document somewhere. The condition logic exist in multiple places so instead of copy/paste on that kind of architectural information, it would be better documented elsewhere outside of the code.
There was a problem hiding this comment.
I think you're right. We probably should add something like How condition works section in the general doc instead of copy/paste it over and over onto every conditional related util function. Anyway, since it's not related to this project anymore, I'm going to merge the PR after the build passes and we can move the discussion to somewhere else if needed.
addon/utils.js
Outdated
| * Gets the sub model associated with the path segment | ||
| * @param {String} pathSegment - path of the segment (must be direct descendant) | ||
| * @param {Object} bunsenModel - starting bunsen model | ||
| * @returns {Object} the submodel or undefined if it doesn't exist |
There was a problem hiding this comment.
@lirwin Glad to see you here.
#PATCH#
CHANGELOG
requiredfields