Better handling of duplicate headers when parsing chunks#1017
Open
stuart-marshall wants to merge 4 commits intomholt:masterfrom
Open
Better handling of duplicate headers when parsing chunks#1017stuart-marshall wants to merge 4 commits intomholt:masterfrom
stuart-marshall wants to merge 4 commits intomholt:masterfrom
Conversation
…ate-header More fix cursor for duplicate header
pokoli
requested changes
Aug 22, 2023
Collaborator
pokoli
left a comment
pokoli
left a comment
There was a problem hiding this comment.
Thanks for your PR. I've added some comments.
It is not clear for me what the new test is doing. IT will be great if you can explain it.
| } | ||
| }, | ||
| complete: function() { | ||
| assert(startsWithEtiamOrLorem); |
Collaborator
There was a problem hiding this comment.
I'm not sure which is the objective of such test.
Could add more assertions? Can we just test the stepped variable to ensure it has steeped correctly?
| }); | ||
|
|
||
| it('Checks cursor when file is large and has duplicate headers', function(done) { | ||
| this.timeout(30000); |
Collaborator
There was a problem hiding this comment.
Why is the timeout required? I think it should be removed.
| if (duplicateHeaders) { | ||
| var editedInput = input.split(newline); | ||
| editedInput[0] = Array.from(headerMap).join(delim); | ||
| // If we change the size of the input due to duplicate headers |
Collaborator
There was a problem hiding this comment.
Could we make the comment smaller?
What about: store the number of removed chars to correctly report meta.cursor ?
| { | ||
| row = rows[i]; | ||
| // use firstline as row length may be changed due to duplicated headers | ||
| if (i === 0 && firstLine !== undefined) { |
Collaborator
There was a problem hiding this comment.
As firstline is no longer used, probably it can be removed as variable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous fix for cursor management when there are duplicate headers worked for the faseMode parse path, but not for the slow mode parse path (that handles quotes).
This change is more general and works for both parse paths.
I also spotted a bug in handling quotes at the end of the file when the headers change.
Added new tests for both of these issues.