Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork754
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
base:master
Are you sure you want to change the base?
Fix key camel case#2177
Conversation
AArnott left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| /// </summary> | ||
| /// <param name="keyBeUpperCamel"></param> | ||
| /// <returns></returns> | ||
| internalstaticstringToLowerCamel(stringkeyBeUpperCamel) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| /// <param name="keyNameCamelCase"> | ||
| /// set keyNameCamelCase=true Convert string type Key to CamelCase rule. | ||
| /// </param> | ||
| publicMessagePackObjectAttribute(boolkeyAsPropertyName,boolkeyNameCamelCase) |
There was a problem hiding this comment.
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)]
There was a problem hiding this comment.
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)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
There was a problem hiding this comment.
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.
AArnott left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
AArnott left a comment
There was a problem hiding this 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} |
There was a problem hiding this comment.
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;} |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
Add KeyName CamelCase