Skip to content

Use data directly when parsing is disabled#6816

Closed
kurkle wants to merge 5 commits intochartjs:masterfrom
kurkle:parsing-disabled-1
Closed

Use data directly when parsing is disabled#6816
kurkle wants to merge 5 commits intochartjs:masterfrom
kurkle:parsing-disabled-1

Conversation

@kurkle
Copy link
Member

@kurkle kurkle commented Dec 7, 2019

Alternative to #6814 and #6815

Master:
image

PR:
image

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this PR also need to update doughnut as I'm doing in #6814?

} else {
parsed = me._parsePrimitiveData(meta, data, start, count);
}
if (parsing !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like updateStacks might not work in the case that parsing === false

Copy link
Member Author

@kurkle kurkle Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsing: false does not affect doughtnut at all (its overwriting the parsing and will probably not have enough data entries to make any difference anyway).
I fixed the optimized (read: not using _getParsed) internal stuff to work.

Also added a test for the data synchronization that I believe #6814 fails in.

@kurkle
Copy link
Member Author

kurkle commented Dec 7, 2019

Fixed the test. @benmccann can you add that to #6814 (and add the array maintenance required to pass it)?

@benmccann
Copy link
Contributor

I added the test to #6814. It passes it. There's no additional array maintenance required. It just replaces the entire array with the user-provided array, so we don't have to do work to keep them in sync

@benmccann
Copy link
Contributor

I think it's a bit confusing that when parsing's on we retrieve data from a different place than when parsing is off and that #6814 is a bit more consistent in this regard

@kurkle
Copy link
Member Author

kurkle commented Dec 7, 2019

I kind of agree. #6814 uses a little more memory and is a bit faster on the _getParsed part.

@benmccann
Copy link
Contributor

Thank you for providing the test!

@etimberg
Copy link
Member

etimberg commented Dec 8, 2019

@benmccann @kurkle between this and #6814 which is preferred?

@benmccann
Copy link
Contributor

@etimberg my preference would be for #6814

@kurkle kurkle closed this Dec 8, 2019
@kurkle kurkle deleted the parsing-disabled-1 branch December 8, 2019 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants