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 ToString methods #437

Merged
merged 9 commits into from
Nov 14, 2024
Merged

Add ToString methods #437

merged 9 commits into from
Nov 14, 2024

Conversation

trumully
Copy link
Contributor

@trumully trumully commented May 14, 2024

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.

trumully added 4 commits May 14, 2024 12:24
Latest macos runner had missing dependencies for testing, macos-12 still has these.
@jamescourtney
Copy link
Owner

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
Copy link
Contributor Author

trumully commented Jun 9, 2024

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
Copy link
Owner

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
Copy link

parched commented Jun 13, 2024

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 trumully#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
Copy link
Contributor Author

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:

  • --generate-methods enabled in the main FlatSharp compilation so only one FlatSharp cs file is generated.
  • --generate-methods enabled in a separate test project similar to the poolable tests.

@jamescourtney
Copy link
Owner

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.

jamescourtney
jamescourtney previously approved these changes Nov 8, 2024
Copy link
Owner

@jamescourtney jamescourtney left a comment

Choose a reason for hiding this comment

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

Sorry for the ridiculously late review. Approved! Please let me know if you need help resolving merge conflicts.

@jamescourtney jamescourtney merged commit 03b052b into jamescourtney:main Nov 14, 2024
5 of 6 checks passed
@jamescourtney
Copy link
Owner

Thanks again for making these changes. Sorry again for the long delay. There is no excuse for me. FlatSharp 7.8 will publish soon with this as the last PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants