Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix key camel case#2177

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
iweiu wants to merge6 commits intoMessagePack-CSharp:master
base:master
Choose a base branch
Loading
fromiweiu:fix-keyCamelCase

Conversation

@iweiu
Copy link

Add KeyName CamelCase

Copy link
Collaborator

@AArnottAArnott left a comment
edited
Loading

Choose a reason for hiding this comment

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

Thanks for contributing!
I have concerns about this change. My feedback is just my thoughts.@neuecc should chime in too, including around whether he likes the idea of this in general.

/// </summary>
/// <param name="keyBeUpperCamel"></param>
/// <returns></returns>
internalstaticstringToLowerCamel(stringkeyBeUpperCamel)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Newtonsoft.Json's camelCase conversion function is far more complicated than this. I don't know why. Maybe it transforms_ThisHere to_thisHere as well. As whatever this function does will be locked in for schema compatibility reasons, we should be mindful about the various prior art in this area and be sure we are comfortable with the design.

Copy link
Author

Choose a reason for hiding this comment

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

[MessagePackObject]
public class Test {
public string Name { get; set; }
public int Age { get; set; }
public string FullName { get; set; }
}

IF [MessagePackObject(keyAsPropertyName = true,keyNameCamelCase =true)]
The current Keys are: name,age,fullName

ELSE IF [MessagePackObject(keyAsPropertyName = true)]
The current Keys are: Name,Age,FullName

ELSE [MessagePackObject(keyNameCamelCase =true)]
The current Keys are: name,age,fullName

@iweiuiweiu requested a review fromAArnottMarch 26, 2025 04:56
/// <param name="keyNameCamelCase">
/// set keyNameCamelCase=true Convert string type Key to CamelCase rule.
/// </param>
publicMessagePackObjectAttribute(boolkeyAsPropertyName,boolkeyNameCamelCase)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for reverting the change to the existing constructor.
I wonder if we need this constructor though. It will require folks to pass in[MessagePackObject(true, true)] to get to the camel case setting. But if you omit this constructor, this syntax is (still) available which IMO reads better:

[MessagePackObject(KeyNameCamelCase = true)]

Copy link
Collaborator

@AArnottAArnottMar 27, 2025
edited
Loading

Choose a reason for hiding this comment

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

Or, if you take my suggestion about enums, a new constructor would work that would be even simpler:

[MessagePackObject(KeyPolicy.CamelCasePropertyNames)]

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Not quite what I was thinking of, but I pushed a change to show my intent.

@iweiuiweiu requested a review fromAArnottMarch 28, 2025 02:48
Copy link
Collaborator

@AArnottAArnott left a comment
edited
Loading

Choose a reason for hiding this comment

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

This is acceptable for dynamic resolver generation at this point. Thanks.
The remaining work is to:

  • ensure the new policy is honored by the source generator path as well.
  • add tests for the new option, for both serialization paths source generation and dynamic object resolver.

@iweiuiweiu requested a review fromAArnottApril 10, 2025 09:41
Copy link
Collaborator

@AArnottAArnott left a comment

Choose a reason for hiding this comment

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

Your latest iteration has some good changes. Thank you.
We still need the source generation side to update to support the new property name transformation.

// {"Foo":10}
Console.WriteLine(MessagePackSerializer.SerializeToJson(newSample3 {Foo=10,Bar=20 }));

// {"foo":10,"bar":20}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would make a more informative sample if the property names were multiple words long, so that those not familiar with camel case can readily see that only the first letter is lowercase and the rest are left alone.

/// Gets a value indicating whether to automatically serialize all internal and public fields and properties using their property name as the key in a map.
/// </summary>
[Obsolete($"Use{nameof(KeyPolicy)} instead.")]
publicboolKeyAsPropertyName{get;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, can't do that. Removing a public property is a breaking change. That's why I added theObsolete attribute instead of removing it.

What wecould do though, is replace it with something that doesn't need to be set in the constructor, like this:

[Obsolete($"Use{nameof(KeyPolicy)} instead.")]publicboolKeyAsPropertyName=>this.KeyPolicyisKeyPolicy.ImplicitPropertyNames orKeyPolicy.ImplicitCamelCasePropertyNames;

hayer reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

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

I think KeyAsPeopleName can be cleaned up before merging the main code.
Using Obsolvete will make the code more chaotic, and concise code will make it clearer to understand.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You seem to think this is up for debate. It is not. Breaking changes are unacceptable in a stable library such as this one. The propertymust remain. The only question is whether to add the[Obsolete] attribute to it. In this case I believe it should, because it is both redundant with the new property and somewhat hazardous for anyone depending on it since it no longer is guaranteed to mean what it used to as a 3rd mode has been added.
And considering almost no one is everreading this property outside of this library, adding Obsolete to it is unlikely to upset anyone.

publicconstintProp1Constant=102;
publicconstintProp2Constant=2223;

publicintProp1{get;set;}=Prop1Constant;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the tests. They'll be more useful if the property names had multiple uppercase letters in them so that we can verify that only the first letter is changed.

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

Reviewers

@AArnottAArnottAArnott requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iweiu@AArnott

[8]ページ先頭

©2009-2025 Movatter.jp