Fix #7722 Improved header validation on CSV import #7950
Conversation
ck-lee
commented
Aug 6, 2016
•
ck-lee
commented
- Fix CSV Import displays errors that cannot be seen on screen #7722
- Added positions and column name when CSV errors on duplicate header name
- Added positions of columns when CSV errors on blank header name
|
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 |
| describe('simple validations', function () { | ||
|
|
||
| it('should return empty array if fields are valid', function () { | ||
| var errors = validateHeaders(['aa', 'bb', 'cc', 'dd', 'ee']); |
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. |
| const errors = []; | ||
|
|
||
| const fieldsWithPositions = _.reduce(fields, (result, field, index) => { | ||
|
|
|
@thomasneirynck could you do a second review? |
|
@Bargs will do! |
ck-lee
force-pushed
the
better-csv-import-errors-messages
branch
2 times, most recently
from
18a9188
to
78e23e0
Compare
| export default function validateHeaders(fields) { | ||
| if (!_.isArray(fields)) { | ||
| throw new Error('first argument must be an array'); | ||
| } |
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. |
|
All done now. @thomasneirynck Please review. I will do a squash when it is ready to be merged. |
|
@ck-lee, thanks! LGTM |
|
LGTM too, @ck-lee squash away and we'll get this thing merged! |
Fix eslint Comment on code Separate validation logic to lib Unit tests Code formatting Use const for immutables and arrays Multiline condition statement with brackets Fix whitespace Added unit tests for argument validations Simplify validateHeaders by using lodash reduce Fix formatting after code review Use arrow function Removed redundant unit tests. Added jsdoc Removed argument validations. Minor comment fix.
ck-lee
force-pushed
the
better-csv-import-errors-messages
branch
from
00c107e
to
49c4d7b
Compare
|
Squashed! Woohoo... This would be my first major PR. Thank you @thomasneirynck @Bargs |
|
Thank YOU @ck-lee, this is a great contribution! |
…messages Fix elastic#7722 Improved header validation on CSV import Former-commit-id: 56b2695
`v95.6.0` ⏩ `v95.7.0` _[Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_ --- ## [`v95.7.0`](https://github.com/elastic/eui/releases/v95.7.0) **CSS-in-JS conversions** - Converted `EuiSelectable` to Emotion ([#7940](elastic/eui#7940)) - Removed `$euiSelectableListItemBorder` - Removed `$euiSelectableListItemPadding` - Converted `EuiSelectableTemplateSitewide` to Emotion ([#7944](elastic/eui#7944)) - Removed `$euiSelectableTemplateFocusBackgroundLight` - Removed `$euiSelectableTemplateFocusBackgroundDark` - Removed `$euiSelectableTemplateSitewideTypes` - Converted `EuiComboBox` to Emotion ([#7950](elastic/eui#7950))
