jamescourtney / FlatSharp

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

Add ToString methods #437

Open trumully opened 4 months ago

trumully commented 4 months ago

This adds tests for the feature originally authored in #417. Let me know if you want any of these tweaked.

Also made some tweaks to the runner versions as macos was missing dependencies that your CI needed.

jamescourtney commented 3 months ago

Thank you for the contribution here. The change looks reasonable. However, since FlatSharp generates all classes as Partial, I'd like to find a way to make this an optional (opt-out?) behavior. It's conceivable that there are people who have defined their own partial classes with their own ToString implementations, and I'd prefer not to break them if I can avoid it.

trumully commented 3 months ago

Maybe we could go about that with an opt-in or opt-out flag in the command-line? Unless there is another avenue for a more suitable solution.

jamescourtney commented 3 months ago

After a bit of deliberation, I'd like to make this opt-in for FlatSharp 7.X, with a switch to opt-in when FlatSharp 8 is released (no ETA on that, but I have promised semantic versioning for FlatSharp, so I try very hard to avoid breaking changes in non-major versions). I can think of three realistic options:

1) As you said, add a command line compiler switch to enable or disable the behavior broadly. This is likely the simpler approach but does sacrifice some granularity.

2) Add an fbs attribute (fs_generate:"ToString"?) that can be applied to all tables/structs to indicate if .ToString should be generated for the individual type.

3) Both of the above. The command line switch could set the default while the fbs annotation could control things with a finer granularity.

===================

The other thing I wonder is the benefit of generating other common methods, such as GetHashCode, ==, !=, .Equals(), sort of how the C# compiler does for record types. I've prototyped generating FlatBuffer objects as records, but there are some behaviors there I'm not happy with. I could imagine:

enum MethodGenerationOptions
{
    None = 0,
    ToString = 1,
    Equality = 2, // !=, ==, .Equals(), .GetHashCode. Maybe split out != and == into a different EqualityOperators category
    All = 3,
}

The usage could be: table MyTable (fs_generateMethods:"All") { ... } and FlatSharp.Compiler.exe --generate-methods ToString;Equality

For the record, I'm not asking you to add all of the other helper methods. However, I'm interested in making this one part of a broader setup. Do you have any feedback on this approach?

===================

Wrapping up, I'm happy to accept a PR contribution here with any of options 1, 2, or 3. Please do make the syntax reasonably generic such as --generate-methods so I can expand on it in the future. There's no expectation from me that you'll build GetHashCode and .Equals. Hope this makes sense. Let me know if I can help or answer questions!

parched commented 3 months ago

I think option 1 will be good to begin with. Ultimately, if FlatSharp were turned into a source generator, it could detect if ToString is already defined by the user in a partial class before generating its own.

Regarding the enum, I think that's good if there's nothing added in the future, but if there is then it might get complicated to generate a subset of the methods. I feel like it will be simpler to just have one flag/attribute per group of methods. E.g., I can image wanting to add Deconstruct too. It means the attributes can be provided with true/false to override the default settings from the command line. e.g., table MyTable (fs_generateEquality:false) { ... }.

We actually have some of the equality code in https://github.com/trumully/FlatSharp/pull/7

For our code base, we treat all struct as value types and some table as reference types and some as value types. I had initially implemented equality for everything, but then realized that wasn't correct for all our tables, so just rolled it back to fix what was causing the warning I was seeing.

trumully commented 3 months ago

I've added a command-line flag --generate-methods that functions as an opt-in for ToString methods (and any other methods that may be added in the future). Right now, it is being tested by generating a separate FlatSharp compilation with the flag enabled.

If you don't like the test done in this way, there are a few other options:

jamescourtney commented 3 months ago

Hi there -- just a quick note that I haven't forgotten about you. I've been ill for the past week (perks of having a toddler) and am not in the right headspace to give this PR the attention it deserves.