jamescourtney / FlatSharp

Fast, idiomatic C# implementation of Flatbuffers
Apache License 2.0
497 stars 50 forks source link

field names are not properly converted to c# coding standard style. #275

Closed shadowbane1000 closed 2 years ago

shadowbane1000 commented 2 years ago

According to the Flatbuffers documentation here https://google.github.io/flatbuffers/flatbuffers_guide_writing_schema.html

"Table and struct field names: snake_case. This is translated to lowerCamelCase automatically for some languages, e.g. Java." and: "Where possible, the code generators for specific languages will generate identifiers that adhere to the language style, based on the schema identifiers."

flatc seems to change the field names for C# to UpperCamelCase, matching most c# style guides. However, FlatSharp does not change the field names at all, so we end up with snake_case fields being used in C#, which feels really weird, and causes an incoherent mix of coding styles. In cases where *.fbs files are shared between languages, it would be great if FlatSharp would perform the same field name transformation as flatc (and protoc, btw).

Expected behavior: given a table definition in a *.fbs file like this: table MyTable { my_long_name:double; } would produce a C# class roughly matching this structure: class MyTable { double MyLongName; }

jamescourtney commented 2 years ago

You're right -- FlatSharp does behave differently in this case. This is going to be a tricky one to resolve without introducing a breaking change.

The simplest thing I can think of would be a FlatSharpNameNormalization property in the .csproj that gets passed down to the Flatsharp compiler, so it's an opt-in feature. I wish FBS supported a attribute syntax like protobuf does that would allows you to specify metadata that applies to an entire FBS file so you could just write it in line, but nothing like that exists to my knowledge.

However, this opens up some questions around what happens in cases like this:

table Foo
{
    snake_case : double;
    SnakeCase : double;
}

Should this be an error? It's technically legal. Perhaps the policy should be that names are only transformed if there is no conflict? Do you know what flatc does?

Aside from the .csproj setting, do you have any alternate ideas for implementation?

shadowbane1000 commented 2 years ago

So flatc issues this warning (I called my fields test_one and TestOne, just to clarify the warning text)

warning: /home/tyler/dev/GDFB/GDFB/mytest.fbs:5: 9: warning: field names should be lowercase snake_case, got: TestOne

The same warning is issued for both C++ and C# code generators. After the warning, it happily generates invalid C# code with the name conflict in it. The C++ code does compile though since the casing rules for C++ leaves it called testOne and TestOne.

I think because you only generate C#, you could catch it and throw an error. But if that is a difficult thing to do, I think letting the compiler catch it isn't too onerous.

I do not have any ideas beside the csproj setting. You might want to consider putting it in as an opt-in for the current version, and changing it to an opt-out for the next breaking-change release.

jamescourtney commented 2 years ago

opt-in for the current version, and changing it to an opt-out for the next breaking-change release.

Yep -- that's exactly what I was thinking.

I think FlatSharp will likely do the same about name conflicts. I try to defer as much validation as I can to the C# compiler. It won't be the best experience, but on par with flatc.

Thanks for raising this. If you'd like to drive the implementation, I welcome contributions. Otherwise, I will take a look as time permits with my day job.

shadowbane1000 commented 2 years ago

Currently my schedule is too tight to make this change, but if that changes, I will let you know. I might get time next week.

jamescourtney commented 2 years ago

I will take care of it this week(end) then. Just wanted to give you the chance to take a crack at it if you were itching to do so :)

jamescourtney commented 2 years ago

279 is open to address this

jamescourtney commented 2 years ago

This will be addressed in the 6.2.0 release, which should be out in the next 2 weeks, depending upon other work that I want to incorporate into the next feature update. All of the preview builds from main do support the opt-in flag to normalize field names.

jamescourtney commented 2 years ago

6.2.0 is published.

jamescourtney commented 1 year ago

326 enables this by default.