Added snake and kebab naming policies to JSON serializer

[YohDeadfall]

Fixes #782

Description

As it was discussed in the issue, I moved code from https://github.com/YohDeadfall/Yoh.Text.Json.NamingPolicies which implements snake and kebab policies.

Customer Impact

After a few years they finally will get these highly demanded features.

Testing

Yes, there are test checking different cases used in previous attempts too, but more of them were added.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

---

Fixes Issue #782

Description

As it was discussed in the issue, I moved code from https://github.com/YohDeadfall/Yoh.Text.Json.NamingPolicies which implements snake and kebab policies.

Customer Impact

After a few years they finally will get these highly demanded features.

Testing

Yes, there are test checking different cases used in previous attempts too, but more of them were added.

Author:	YohDeadfall
Assignees:	-
Labels:	area-System.Text.Json, new-api-needs-documentation
Milestone:	-

[teo-tsirpanis]

@YohDeadfall CI fails with

##[error]src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs(1498,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace

[YohDeadfall]

Okay, it should be fine now:

• Types are explicit everywhere as @gfoidl reminded me;
• Bug with slicing is fixed (forgot to subtract an index while switching from indexer slicing).

[YohDeadfall]

@teo-tsirpanis, while the public API shapes is awaiting for an approval, internals can be reviewed even now.

[NinoFloris]

Some feedback, the code is a bit brittle at the moment.

I rewrote WriteWord to something a bit more resilient, feel free to copy it over.
https://gist.github.com/NinoFloris/38597a90fb7c422d0244098c6fd94809

Also I noticed newlines are left alone entirely?

[NinoFloris]

How does this fit with custom boundary chars?

[YohDeadfall]

Hello, Nino (: Thank you for the valuable review.

That's a trimmed version of the original implementation which used word boundary and uppercase-lowercase logic to get words from an input string, but it was too heavy for a name policy as @ericstj told me in the previous attempt or in the issue. Therefore, the current implementation only support CLR member names and not any input string.

[ericstj]

I don't recall the exact conversation but it may have been about bringing in new library dependencies to handle word boundaries? Ultimately my guidance is usually just a suggestion and not a hard requirement -- we make the hard calls in API review and ultimately in PR with the area owners. Not everything is settled and if there is new data that invalidates past decisions it can be revisited. We do prioritize the right extensibility points in JSON rather than implementing every feature. cc @dotnet/area-system-text-json

[NinoFloris]

What works for the BCL in span.ToLower/ToUpper works here but a comment stating that we don't expect the length to change after upper/lowercasing, so we can just use length here, would be good.

[YohDeadfall]

I would like to avoid assumptions on how internally such things work. Any dependency can be changed, but the code must continue to work.

[NinoFloris]

I agree which is why my gist code doesn't rely on it :)

[eiriktsarpalis]

I think you can make the assumption that length can't change:

runtime/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.Globalization.cs

Lines 275 to 289 in f2de14d

	public static int ToUpperInvariant(this ReadOnlySpan<char> source, Span<char> destination)
	{
	if (source.Overlaps(destination))
	throw new InvalidOperationException(SR.InvalidOperation_SpanOverlappedOperation);
	// Assuming that changing case does not affect length
	if (destination.Length < source.Length)
	return -1;
	if (GlobalizationMode.Invariant)
	InvariantModeCasing.ToUpper(source, destination);
	else
	TextInfo.Invariant.ChangeCaseToUpper(source, destination);
	return source.Length;
	}

cc @GrabYourPitchforks

[YohDeadfall]

Sure, but if that part changes at some point for whatever reason, the converter must be fixed too. Would someone remember to do that? I guess, not.

[eiriktsarpalis]

Do we have evidence suggesting that the UTF-16 spec could permit variable character lengths when mapping casings? This would break the ToUpper/ToLower methods on char.

cc @dotnet/area-system-globalization

[tarekgh]

For ToUpper/ToLower on char, we always map it manually through caching some tables from ICU. So, we guarantee 1-to-1 mapping. For the overloads that takes string, it is possible to get variable length.

[krwq]

@YohDeadfall given the newer APIs haven't been reviewed yet I'll refrain from review for now and let's not go too far with implementation or testing to not do unnecessary work.

We've also discussed the new APIs during our JSON triage meeting and we don't feel strongly about either having or not having lowercase/uppercase APIs so we'll let API review folks make the final call. Specifically we don't have enough proof to believe uppercase version will be useful or not useful for anyone and at the same time the APIs seem to be isolated enough that we're ok with having them.

[deeprobin]

Thank you for your PR. It already looks very good. Have noted a few small things.

Looking forward to seeing this change with .NET 8.

[YohDeadfall]

@eiriktsarpalis, I've addressed all name issues except one with which I strongly disagree (result and resultLength). In #69613 (comment) you can see the result of passing variables to static local methods instead of using a state struct which captures them. Therefore, it's ready for the final review, I guess.

[krwq]

I'm curious - should we be using identical rules to CamelCase except also adding - or _?

[YohDeadfall]

Sure, I can do that, but knowing about "compatibility concerns" I expect to hear "no" from other reviewers (:

[YohDeadfall]

And actually, what you asked is how naming policies work in other projects and platforms. They always split the input and then transform each word and concatenate them back.

[krwq]

Please file an issue to consider joining this code with CamelCasing and I think we're ready to merge after that

[krwq]

#77262

[krwq]

LGTM with question

[MartyIX]

Any chance of this getting into .NET 7 via servicing?

[YohDeadfall]

Guess, no, but feel free to use my package which source was used for that PR and later adjusted to address review issues from here:

• https://github.com/YohDeadfall/Yoh.Text.Json.NamingPolicies
• https://www.nuget.org/packages/Yoh.Text.Json.NamingPolicies

I'll release version 1.0.0 soon.

[krwq]

I doubt that will meet servicing bar. System.Text.Json ships as nuget package so you should be able to consume it with the next preview or nightly.

[eiriktsarpalis]

We should probably add a few tests validating that the output matches that of the Json.NET implementation, particularly when it comes to nontrivial inputs. We have time until .NET 8 ships, so we can always introduce breaking changes if needed.

[SaphireLattice]

Is there a particular reason why JsonSeparatorNamingPolicy is an internal class that is not exposed? I figure it could be rather useful in rare odd cases? Unless there's something I misunderstand about marking things internal.

[YohDeadfall]

That's pretty simple to answer. That type is an implementation detail which shall not be exposed especially in case when it's a part of the SDK where anything public is written in stone and cannot be changed. Odd scenarios aren't a thing which is covered in developer kits, platforms and runtimes.