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.

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

Rework SnakeCase/KebabCase naming policies to closer match Json.NET's #90316

Merged
merged 12 commits into from
Aug 11, 2023

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Aug 10, 2023

This PR:

  1. Updates the JsonSeparatorNamingPolicy so that non-alphanumeric characters are not being truncated, minimizing the probability of naming collisions.
  2. Extends unit test coverage for the component.

After a lot of discussion we concluded that:

  1. The policies should keep the fixes for StringUtils.ToSnakeCase fails for many common scenarios JamesNK/Newtonsoft.Json#1956, resulting in some deviation from Json.NET semantics. We need to document these where appropriate.
  2. We will not be adding support for surrogate pairs. Surrogate pairs are handled gracefully by the current implementation, however uppercase surrogate pairs are not recognized as such. This matches the implementation of JsonNamingPolicy.CamelCase. We should consider adding surrogate pair support in the future, although that would require addressing Proposal: Create OOB package for Rune, System.Text.Unicode.Utf8, and related APIs #52947.

Fix #77309.

@ghost
Copy link

ghost commented Aug 10, 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

As an alternative to #90248, this PR reworks the naming policies so that non-alphanumeric characters are never being trimmed or replaced, while still making sure that this Json.NET bug is addressed.

Fix #77309.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@stephentoub
Copy link
Member

I just left comments on #90248. What's the difference between this one and that?

@YohDeadfall
Copy link
Contributor

I like it much more than the previous attempt, and it fits into the survey results which isn't representative at the moment. There are two votes on Mastodon for keeping punctuation, but another one from Twitter is for trimming. With the first question things are much better and there are 86% of votes for partial compatibility with JSON.NET on Mastodon and 50% on Twitter (where just two people voted).

@YohDeadfall
Copy link
Contributor

What's the difference between this one and that?

The difference is in how punctuation is treated: #90248 (comment). This pull request behaves more naturally.

@eiriktsarpalis eiriktsarpalis changed the title Rework SnakeCase/KebabCase naming policies so that non alphanumeric characters are not being trimmed Rework SnakeCase/KebabCase naming policies to closer match Json.NET's Aug 10, 2023
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Added a few questions, LGTM otherwise.

@eiriktsarpalis eiriktsarpalis merged commit c6db89c into dotnet:main Aug 11, 2023
103 checks passed
@eiriktsarpalis
Copy link
Member Author

@YohDeadfall thank you for providing useful feedback!

@eiriktsarpalis eiriktsarpalis deleted the snake-case-alternative branch August 11, 2023 11:18
@eiriktsarpalis eiriktsarpalis restored the snake-case-alternative branch August 11, 2023 11:25
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate that the new snake/kebab case naming policies match the Json.NET implementation.
4 participants