normalizes the calls to valueof to three arguments #1670
Closed
Conversation
This file contains 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
…*, and *data* previously *data* was present in some cases (when going through the *arraytype*.map code path) but not others (e.g. when doing type conversion) Addresses this comment in #1665 (comment)
|
closing as it doesn't seem necessary |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Our types don't guarantee the presence of the third argument to valueof—it depends on whether some type conversion happens, which is an arbitrary implementation choice.
this PR normalizes the calls to valueof to three arguments: element, index, and data
previously data was present in some cases (when going through the arraytype.map code path) but not others (e.g. when doing type conversion)
Addresses this comment in #1665 (comment), guaranteeing that you can use a function filter(d, i, data) { return i < data.length-1 } to select any value but the last.
This seems a bit more robust and I don't expect any performance impact (the wrapper is invoked only when necessary). That said I'm not sure we should do this, or go the other way and ban the third argument—or ignore the issue altogether.