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 Orleans support #653

Merged
merged 13 commits into from
Aug 6, 2024
Merged

Add Orleans support #653

merged 13 commits into from
Aug 6, 2024

Conversation

sandervanteinde
Copy link
Contributor

Adds orleans support to Vogen.

This is my first time contributing to an open source project, so I'm not fully familiar with how it should be done. Any help on what still needs to be done to fully integrate this would be greatly appreciated!

Should fix #385

@SteveDunn
Copy link
Owner

Adds orleans support to Vogen.

This is my first time contributing to an open source project, so I'm not fully familiar with how it should be done. Any help on what still needs to be done to fully integrate this would be greatly appreciated!

Should fix #385

Thank you very much for the PR! This will make a big difference to everyone that's been patiently waiting for this feature!

You've done a great job navigating around and finding the right places to put things. When Vogen started, the converters needed private access to the value object, e.g. _value etc. With more recent versions of C#, we can use new functionality (namely UnsafeAccessor) that was designed for source generators that lets them access private stuff from outside of the class. An example is the new BSON serialization. It means we can now create these serializers in separate classes that don't need to be inner classes.

I'm happy to jump in and rearrange this. While I'm there, I'll also try to make the structure clearer to make similar things easier to add in the future.

Thanks again for the contribution - let me know if you're happy for me to jump and rearrange to the new style.

@sandervanteinde
Copy link
Contributor Author

Thanks again for the contribution - let me know if you're happy for me to jump and rearrange to the new style.

Of course! Feel free to put the dots on the i ;-) I'll monitor this PR regardless in case you need Orleans-specific inputs from my end.

I have another suggestion that is unrelated to this PR though, I noticed that Snapshots can sometimes change just because of whitespace changes, while acceptable (there's a different output) I do have a recommendation that I'd like to put forward:

In WriteWorkItems.cs:38 there's a piece of code that parses the SourceText.

If you add these prior to that, it will do some very neat auto-formatting based on Microsoft's own parsing engine:

SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(classAsText);
SyntaxNode root = syntaxTree.GetRoot();
SyntaxNode formatted = root.NormalizeWhitespace();

SourceText sourceText = SourceText.From(formatted.ToFullString(), Encoding.UTF8);

This might reduce the amount of times snapshots changes (and as a bonus, it formats all source generated output ;-) )

@SteveDunn
Copy link
Owner

Thanks again for the contribution - let me know if you're happy for me to jump and rearrange to the new style.

Of course! Feel free to put the dots on the i ;-) I'll monitor this PR regardless in case you need Orleans-specific inputs from my end.

I have another suggestion that is unrelated to this PR though, I noticed that Snapshots can sometimes change just because of whitespace changes, while acceptable (there's a different output) I do have a recommendation that I'd like to put forward:

In WriteWorkItems.cs:38 there's a piece of code that parses the SourceText.

If you add these prior to that, it will do some very neat auto-formatting based on Microsoft's own parsing engine:

SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(classAsText);
SyntaxNode root = syntaxTree.GetRoot();
SyntaxNode formatted = root.NormalizeWhitespace();

SourceText sourceText = SourceText.From(formatted.ToFullString(), Encoding.UTF8);

This might reduce the amount of times snapshots changes (and as a bonus, it formats all source generated output ;-) )

Great stuff! I'll get to shortly. Thanks for the tip re. formatting! That will certainly save time and effort in trying to align code (to be honest, I gave up trying to align code long ago, as you can probably tell from the mess that is generated! :)

@sandervanteinde
Copy link
Contributor Author

sandervanteinde commented Aug 5, 2024

Thanks again for the contribution - let me know if you're happy for me to jump and rearrange to the new style.

Of course! Feel free to put the dots on the i ;-) I'll monitor this PR regardless in case you need Orleans-specific inputs from my end.
I have another suggestion that is unrelated to this PR though, I noticed that Snapshots can sometimes change just because of whitespace changes, while acceptable (there's a different output) I do have a recommendation that I'd like to put forward:
In WriteWorkItems.cs:38 there's a piece of code that parses the SourceText.
If you add these prior to that, it will do some very neat auto-formatting based on Microsoft's own parsing engine:

SyntaxTree syntaxTree = CSharpSyntaxTree.ParseText(classAsText);
SyntaxNode root = syntaxTree.GetRoot();
SyntaxNode formatted = root.NormalizeWhitespace();

SourceText sourceText = SourceText.From(formatted.ToFullString(), Encoding.UTF8);

This might reduce the amount of times snapshots changes (and as a bonus, it formats all source generated output ;-) )

Great stuff! I'll get to shortly. Thanks for the tip re. formatting! That will certainly save time and effort in trying to align code (to be honest, I gave up trying to align code long ago, as you can probably tell from the mess that is generated! :)

It's not like Microsoft made it particularly easy to write source generators... Took me a while to figure out the code I shared above haha

Edit: typos

@SteveDunn
Copy link
Owner

Hopefully, this should be finished building soon, and be ready for merging. Could you think of a trivial example app to put in the Samples folder? There's a few in there already, like an ASPNET app, and some examples of Onion/Ports & Adapters etc.

@sandervanteinde
Copy link
Contributor Author

sandervanteinde commented Aug 6, 2024

Hopefully, this should be finished building soon, and be ready for merging. Could you think of a trivial example app to put in the Samples folder? There's a few in there already, like an ASPNET app, and some examples of Onion/Ports & Adapters etc.

Added an example, had to add this to the csproj, because the build was failing on these warnings and I couldn't figure out as to why that was.

<NoWarn>$(NoWarn),NU1603,NU1903</NoWarn>

Not sure why the build was failing, maybe you have some insight on that?

@SteveDunn
Copy link
Owner

Hopefully, this should be finished building soon, and be ready for merging. Could you think of a trivial example app to put in the Samples folder? There's a few in there already, like an ASPNET app, and some examples of Onion/Ports & Adapters etc.

Added an example, had to add this to the csproj, because the build was failing on these warnings and I couldn't figure out as to why that was.

<NoWarn>$(NoWarn),NU1603,NU1903</NoWarn>

Not sure why the build was failing, maybe you have some insight on that?

Thank you! The samples use a local nuget folder and consume the very latest nuget package of Vogen (version 999.99.xxx). I can see that you've used the other .csproj files as an example, so that's great! 👍

@SteveDunn
Copy link
Owner

This looks great! Thanks again for your contribution

Copy link
Owner

@SteveDunn SteveDunn left a comment

Choose a reason for hiding this comment

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

LGTM!

@SteveDunn SteveDunn merged commit 0d37a05 into SteveDunn:main Aug 6, 2024
6 checks passed
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.

[feature request] Orleans integration
2 participants