Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend Utf8JsonReader.CopyString to also accept JsonTokenType.Number values #84375

Open
eiriktsarpalis opened this issue Apr 5, 2023 · 2 comments
Open

Extend Utf8JsonReader.CopyString to also accept JsonTokenType.Number values #84375

eiriktsarpalis opened this issue Apr 5, 2023 · 2 comments
Assignees
@eiriktsarpalis
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone
Future

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Apr 5, 2023 •
edited
Loading

The current implementation of the Utf8JsonReader.CopyString methods only accepts JSON string tokens, that is JsonTokenType.PropertyName and JsonTokenType.String. When implementing JsonConverters for large numeric types such as BigInteger (#60780) or Int128/UInt128 (#73500) extending the method to also handle JSON number tokens would greatly simplify getting allocation-free views of underlying JSON number encodings.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 5, 2023
@ghost
Copy link

ghost commented Apr 5, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

The current implementation of the Utf8JsonReader.CopyString methods only accepts JSON string tokens, that is JsonTokenType.PropertyName and JsonTokenType.String. When implementing JsonConverters for large numeric types such as BigInteger (#60780) or Int128/UInt128 (#73500) extending the method to also handle JSON number tokens would greatly simplify getting allocation-free views of the JSON numbers.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json
Milestone: -

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Apr 5, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Apr 5, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Apr 5, 2023
@eiriktsarpalis eiriktsarpalis mentioned this issue Jun 24, 2023
Closed
@jozkee jozkee mentioned this issue Jul 16, 2023
Merged
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jul 16, 2023
@eiriktsarpalis
Copy link
Member Author

Following the discussion in #88962 (comment) we have concluded that even though the functionality is useful, there is something offputting about a method called CopyString working with number tokens (even though JSON numbers are technically still encoded as arbitrarily long UTF-8 strings). We should consult with the API review board first, considering potential alternative method names such as CopyValue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Milestone
Future
Development

No branches or pull requests
1 participant
@eiriktsarpalis