Fix #7722 Improved header validation on CSV import#7950
Conversation
|
Can one of the admins verify this patch? |
|
Screenshot for reference... |
|
@ck-lee Thanks for the contribution! I'll try to review this as soon as I can |
There was a problem hiding this comment.
Use const here, and if you need mutability prefer let over var. This applies to the whole file, but I'll just leave this one note here
|
@ck-lee this looks great! I really appreciate the thorough tests you included. I added some comments, but it's all pretty minor stuff |
|
@Bargs: Thanks again. Please review. |
|
@thomasneirynck could you do a second review? |
|
@Bargs will do! |
18a9188 to
78e23e0
Compare
There was a problem hiding this comment.
I'd remove this guard clause. This isn't really public API for 3rd party clients, where throwing runtime errors can be helpful.
Some clean JSDoc on the function signature would tell the story just as well IMO, and it'd be less code.
There was a problem hiding this comment.
I disagree with that, I still think it's a good idea to enforce your preconditions. Especially when using lodash where many of the functions can operate on both objects and arrays, not checking your arguments could lead to hard to find bugs that don't throw errors.
That said, it's not in our styleguide and it's a minor issue here, so it's not worth holding up this PR.
There was a problem hiding this comment.
Yeah. This is a minor one. And I think it is worth considering in the future if the validateHeaders module grows more than a couple validation checks.
|
great! minor suggestions to reduce code footprint, but LGTM. |
Uh oh!
There was an error while loading. Please reload this page.