add multiple column delimiter support #291
Conversation
wdavidw
left a comment
wdavidw
left a comment
There was a problem hiding this comment.
Tests looks good, code shall be simplified by always having delimiter as an array
|
Thanks for the tip. I've refactored the code as you mentioned. Please take a look in the new commit. |
lib/index.js
Outdated
| `got ${JSON.stringify(options.delimiter)}` | ||
| ]) | ||
| } | ||
| let delimiter = options.delimiter |
There was a problem hiding this comment.
Let's use options.delimiter everwhere, there is no need to confuse the code reader with a new variable which is just the same as another one.
lib/index.js
Outdated
| ]) | ||
| } | ||
| let delimiter = options.delimiter | ||
| let delimiter_error = new CsvError('CSV_INVALID_OPTION_DELIMITER', [ |
There was a problem hiding this comment.
If the error is thrown in 2 places, then I create 2 distincts errors, I can't take care of this later. It is good that you check the array length, I was thinking about this as well.
There was a problem hiding this comment.
okay I will add 2 distinct errors
There was a problem hiding this comment.
The reason I did this is because I needed access to the original delimiter option for throwing it inside the loop. If i change the delimiter to array, then I lose access to the original one and hence the error generated does not match the expected error.
|
Everything looks good on my side, do you see anything else to do ? If not, could you squash your changes into a single commit? |
|
yeah sure. I will check it again and squash the commits into a single commit |
…de-csv-parse into multiple-column-delimiters
|
I think that's all. Please review the squashed commit. |
|
Thank you very much for your contribution! |
2 participants
Please take a look and tell me if there's any issues.