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

Add JsonPascalCaseNamingPolicy #100017

Closed
wants to merge 6 commits into from

Conversation

mithileshz
Copy link

@mithileshz mithileshz commented Mar 20, 2024

Adding a Pascal Case Naming Policy for System.Text.Json.

Fixes #34114

I was exploring the code to get a feel for it and thought I'd give it a go. Please let me know if there are any changes needing to be made!

Copy link

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, 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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2024
@mithileshz
Copy link
Author

@dotnet-policy-service agree

{
public override string ConvertName(string name)
{
if (string.IsNullOrEmpty(name) || !char.IsLower(name[0]))
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this just mirrors the behavior of the camel case converter, but shouldn't we also try to have these convert kebab-case and snake_case renderings to PascalCase? What does Json.NET do in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

In my honest opinion all converters should behave like kebab and snake case ones, i.e. split an input string into parts, modify them and concatenate. That's exactly how the rest of the world does that, but for historical reasons the camel case policy cannot be changed. Due to that reason I'm thinking that this implementations sounds good, it just mirrors what the snake case does. That's the most expected behavior, I think.

But if it's possible to align the behavior of both camel and pascal case policies to what's going on for snake and camel case policies, then better to do so.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the same thing, it might be something we could turn on with a feature flag in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Hey both, sorry for the late replies.

Json.NET doesn't have an implementation for PascalCase exactly, it just returns the name of the property as is in the DefaultNamingStrategy - see here

There is an implementation of CamelCaseNamingStrategy - here
and an implementation of KebabCaseNamingStrategy - here
These call the StringUtils which also doesn't have an implementation for PascalCase - here

I can look into the current kebab and snake case scenarios and try implement that in this PR for the Pascal Case scenario if that works? I might need some help on how to create feature flags as it would be changing the existing camel case scenarios which I guess would be a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with the pascal case policy is that it was released as a part of System.Text.Json and therefore does things very primitively. The kebab and snake policies were added long time after a research I made on how other frameworks handle it outside of .NET world. The origin of these two policies is https://github.com/YohDeadfall/Yoh.Text.Json.NamingPolicies where later today I will push pascal and camel case policies which are done well. There are some deviations from what the SDK since @eiriktsarpalis found a bug which I didn't backported, but that's in plans too. Then that code can be used as is in the SDK once again, but for backward compatibility the current implementation of the camel case policy must be kept untouched. In addition to it a similar policy should be added which will upper case the first word (reverse behavior of the current camel case policy). That "reverse" camel case policy is exactly what you need at the moment.

I might need some help on how to create feature flags as it would be changing the existing camel case scenarios which I guess would be a breaking change?

That goes to @eiriktsarpalis. Perhaps, it needs approval from the .NET design review team first.

@eiriktsarpalis
Copy link
Member

@mithileshz thanks for contributing this PR, it looks excellently implemented! Because this is adding new APIs, please note that before we can review or merge this PR an API proposal must be submitted for review (see our API review process document for more details).

Essentially, this would involve adding a proposal to the parent issue (#34114) showing the new API surface that is being added (essentially showing the diff of your changes in the ref source file plus a couple of basic usage examples). Would you be able to add a proposal like that? Thanks.

@eiriktsarpalis eiriktsarpalis added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2024
@mithileshz
Copy link
Author

@eiriktsarpalis of course. Thank you for the information. I wasn't aware as I picked it up just out of curiosity so I'll do that right away.
Thank you for the feedback!

@elgonzo
Copy link

elgonzo commented Mar 22, 2024

I wonder what the expected/intended behavior should be with regard to all-upper case strings/starting words like "UPPER".
Is pascal case conversion supposed to produce "Upper" or "UPPER"?

The camel case policy converts "UPPER" to "upper", changing the letter case not just for the 1st character, but for the whole word. Because camel case and pascal case are typically supposed to be the same except for the first letter, i would therefore intuitively expect the pascal case policy to convert "UPPER" to "Upper", but the current implementation here leaves the word all-upper case.

@YohDeadfall
Copy link
Contributor

Is pascal case conversion supposed to produce "Upper" or "UPPER"?

I don't remember the exact rule for the camel case policy, but that one shall follow the same logic. Currently it just touches the first character as it was originally implemented by the camel case policy, but later it was improved to handle abbreviations like IO. That should be done in the current pull request too.

@mithileshz
Copy link
Author

I don't remember the exact rule for the camel case policy, but that one shall follow the same logic. Currently it just touches the first character as it was originally implemented by the camel case policy, but later it was improved to handle abbreviations like IO. That should be done in the current pull request too.

Hey @YohDeadfall, would you be able to point me towards where the abbreviations are set up? As the Camel Case Naming Policy currently is very basic and ToLower's the character based on the next character.

@eiriktsarpalis
Copy link
Member

Let's convert this to a draft while we wait on API review.

@eiriktsarpalis
Copy link
Member

The API has now been approved but please note that main is now frozen when it comes to adding new features. We can revisit this PR after mid-August when the .NET 9 branch has been snapped from main.

@eiriktsarpalis
Copy link
Member

@mithileshz would you like to give this another go? .NET 9 has now been snapped from main so we can accept the change now.

@mithileshz
Copy link
Author

@eiriktsarpalis yes please. Can I get a bit of a guide on this? I have read through the comments and just to confirm - we want something similar to the rest of the APIs where it converts from one type to another. Rather than something similar to the camelCase one where it only takes the first character?

@eiriktsarpalis
Copy link
Member

That's correct. It should more closely follow the recently added kebab case and camel case converters that tokenize identifiers and then re-splice them together using PascalCase conventions.

@YohDeadfall
Copy link
Contributor

Then I guess, it's not different from having a base class with a virtual method overridden for exact implementations. You can see the same approach in https://github.com/YohDeadfall/Yoh.Text.Json.NamingPolicies/blob/v1.1.2

@eiriktsarpalis eiriktsarpalis reopened this Sep 9, 2024
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 9, 2024

@YohDeadfall how are tokens being split in your pascal case policy? Is casing the only determinant or does it also recognize snake/kebab cased inputs?

EDIT Looking at your README seems to have answered my question. The examples listed should be a good guide I think.

@YohDeadfall
Copy link
Contributor

My logic is a bit different than yours because I simply decided to give no shit about the legacy (: If you didn't change the string splitting rules too much to support JSON.NET, then the only thing needed here is a virtual method and a base class. That's due to being close to Unicode segmentation rules, but simplified them a lot just to cover identifiers only. But originally I used an implementation of the Unicode segmentation logic as the heck crate does in Rust.

Other ecosystems outside use regular expressions, perhaps it's JS or Python, don't remember. You can find all of that in my original unmerged pull request for kebab and snake cases. There were a lot of info on the topic.

@eiriktsarpalis eiriktsarpalis removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 11, 2024
Copy link
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@jai-dewani
Copy link

After reading all the discussion on the PR, I realized this can be easily achieved by using the same abstract base class that tokenize identifiers for Snake and Kebab Case

namespace System.Text.Json
{
    internal sealed class JsonPascalCaseNamingPolicy : JsonSeparatorNamingPolicy
    {
        public JsonPascalCaseNamingPolicy ()
            : base(lowercase: false, separator: '')
        {
        }
    }
}

This ensures that Pascal Case is in sync with Snake and Kebab case since they would share the base class which handles the splicing of words and mering them back.

Let me know if I have maybe glossed over some key scenarios or discussion since this is my first attempt at trying this out.

@elgonzo
Copy link

elgonzo commented Oct 20, 2024

@jai-dewani

What is separator: '' supposed to be? ;-)

@jai-dewani
Copy link

Hello @elgonzo

JsonSeparatorNamingPolicy.cs which is an internal class in System.Text.Json namespace. It accepts a separator in it's constructor which is used to join back the tokenized words as the final result .
In Kebab case, the separator is - since all words are separated and then joined back with - in between.
Same goes for Snake case with the difference being the separator is _

One more thing I just now noticed, the first argument in the constructor of JsonSeparatorNamingPolicy which is called lowercase handles the upper/lower case of all character, not just the first word. So we might have to change this to an enum instead of a bool, with possible values

ALL_CHARACTER_UPPERCASE,
ALL_CHARACTER_LOWERCASE,
FIRST_CHARACTER_UPPERCAE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[System.Text.Json] Add JsonPascalCaseNamingPolicy.
5 participants