Refactor dotnet code gen #233

clrudolphi · 2024-06-28T18:03:17Z

🤔 What's changed?

Rewrote the Dotnet code generation script to adopt the ruby erb gem approach used for other languages.
Modified the target .NET Framework settings by eliminating the use of .NET45 - targeting .NETStandard2.0 only.

Used a DevContainer so you'll see a devcontainer.json in the file set.

⚡️ What's your motivation?

To bring the .NET implementation to parity with the java version.

🏷️ What kind of change is this?

🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

Per the question I posted on issue #230, does the ordering of properties of a entity matter when serialized? The serialization I have does not match the sequence of properties shown in the CCK examples.

I've used a Microsoft provided devcontainer image. Do you mind? Or should that be changed to the github image?

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- [X ] I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
[X ] Users should know about my change
- [X ] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

…ttributes will work.

…/messages into RefactorDotnetCodeGen

Reverted to .NET Standard 2.0, Passing simple test.

mpkorstanje

Looks good over all.

But I'm a bit puzzled as to why the CI hasn't been activated. I would have expected some build failures. I'll look into that.

For the mean time, I left a few comments. The first two are for myself. The others you may want to address.

dotnet/.github/dependabot.yml

dotnet/Cucumber.Messages.Specs/Cucumber.Messages.Specs.csproj

dotnet/Makefile

mpkorstanje · 2024-06-28T19:17:39Z

dotnet/Cucumber.Messages.Specs/MessagesSpec.cs

+            var jsonOptions = new JsonSerializerOptions();
+            jsonOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
+            jsonOptions.Converters.Add(new CustomEnumConverter<StepDefinitionPatternType>());
+            jsonOptions.DefaultIgnoreCondition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingNull;


These json options are non trivial and will have to be replicated by anyone who wants to serialize messages. So I think it would be prudent to provide a serializer and deserializer class as part of the messages library. This should hopefully prevent any mistakes from taking hold.

You may also want to look at some of the other utility classes that come with messages.

https://github.com/cucumber/messages/tree/main/java/src/main/java/io/cucumber/messages

When comparing do note that unlike many other languages, Java does not have an out of the box json serializer On the other hand, the libraries Java does have update quite frequently. To avoid this cascading down from messages, the MessageToNdjsonWriter is constructed with a Serializer interface instead. This allows us to keep the serializer library at the edge of the dependency tree.

Thanks for the quick feedback. I'll take a look at whether the utility classes make sense in the dotnet ecosystem.
I'll double check with Gaspar about the .net version (5 v 8).
And I'll port over the Java test suite (or something similar).

Completed. Please take a look at what was added to the support classes to make serialization easier. It aligns with how the Java implementation works.

mpkorstanje · 2024-06-28T19:27:23Z

Per the question I posted on issue #230, does the ordering of properties of a entity matter when serialized? The serialization I have does not match the sequence of properties shown in the CCK examples.

Answered: #230 (comment). The order does not matter.

I've used a Microsoft provided devcontainer image. Do you mind? Or should that be changed to the github image?

As long as that choice falls within the common practices in the .Net ecosystem that is okay. The goal is to make sure the next contributor familiar with .Net understands what is going on.

I also don't think the Github provided containers are available on Dockerhub.

… for serialization and TimeStamp handling. Added tests.

clrudolphi · 2024-06-30T01:20:29Z

Updated the PR to include a ChangeLog entry.
Revised the code gen script to house all the generated C# classes in a /generated folder.
Added support classes in alignment with what the Java implementation has (for serialization support and TimeStamp conversions).
Added tests for the support classes (again, similar in nature to what the Java implementation does)
Dropped target framework support for .NET Framework 4.x (leaving .NET Standard 2.0 as the target).

clrudolphi · 2024-06-30T01:23:15Z

@mpkorstanje please add @gasparnagy as a reviewer of this PR. Thanks

mpkorstanje · 2024-06-30T08:38:38Z

.github/workflows/test-dotnet.yml

@@ -22,5 +22,6 @@ jobs:

      - name: run tests
        run: |
-          make
+         echo "No tests yet"
+         exit 1


Got CI to trigger properly. Please add something sensible here.

i've modified that to invoke dotnet test

dotnet/Cucumber.Messages.Specs/ProtocolVersionTest.cs

… testing step. (Updated minimum dotnet version from 5.x to 8.x) Restored default.mk in the dotnet folder (was mistakenly deleted in an earlier commit)

gasparnagy

Thx for the code, it looks good.

I added my notes. Most of them minor, there are only a few tricky questions.

dotnet/Cucumber.Messages/Cucumber.Messages.csproj

dotnet/Cucumber.Messages.Specs/ProtocolVersionTest.cs

dotnet/Cucumber.Messages/ProtocolVersion.cs

dotnet/Cucumber.Messages/generated/Attachment.cs

jsonschema/scripts/codegen.rb

…n: a) whether to use long or int, b) whether to take a dependency on System.Text.Json package or find another option.

luke-hill · 2024-07-02T10:30:25Z

Dependent on the timeframes involved this will maybe require a large conflict resolution as I'm in the process of refactoring the codegen (Only a small refactor, but lots of moving of files around), here: #228

I'm currently at a bit of an impasse in terms of contributing for the coming weeks, so if this is merged in in the next 1-2 weeks you'll have no issues and I'll fix up conflicts my end. But just letting you know 👍

…he PR. Put a lock around the creation of the json serialization options singleton to address what appear to be threading issues while running tests during github CI.

dotnet/Cucumber.Messages/NdjsonSerializer.cs

mpkorstanje · 2024-07-02T14:07:33Z

@luke-hill cheers.

@clrudolphi @gasparnagy take your time, I won't be looking at opensource this week until the end of the weekend.

dotnet/Cucumber.Messages/CucumberMessagEnumConverter.cs

mpkorstanje · 2024-07-05T15:18:38Z

dotnet/Cucumber.Messages/NdjsonSerializer.cs

+            return NdjsonSerializer.Deserialize<Envelope>(json);
+        }
+
+        public static T Deserialize<T>(string json)


I would only ever expect users of this API to read and write envelopes. So Serialize<T> and Deserialize<T> can probably be package private.

I had left it public in order to test it (via elements more granular than Envelopes). Now that we're over that hurdle, I'll remove the single such test and make these private.

clrudolphi · 2024-07-05T22:14:44Z

I've also added a line to MessagetoNdjsonReader and Writer classes to Close their internally held streamwriter upon Dispose().

…/messages into RefactorDotnetCodeGen

luke-hill

Popped some linting suggestions. I've tried to keep them brief / relevant.

I think it's probably best we get this merged in first and then merge my codegen stuff in after as there's still a lot of stuff to tidy up but probably best I tackle it.

dotnet/.github/dependabot.yml

dotnet/Cucumber.Messages.Specs/BasicMessageSerializationTests.cs

dotnet/Cucumber.Messages.Specs/MessagesTest.cs

dotnet/Cucumber.Messages.Specs/NdjsonStreamSerializationTests.cs

jsonschema/scripts/templates/dotnet.dotnet.erb

dotnet/Cucumber.Messages/CucumberMessagEnumConverter.cs

clrudolphi · 2024-07-09T13:01:49Z

Thanks for your review comments so far.
I think the only outstanding item is the comment from @luke-hill about the dotnet generation script and the complexity required when declaring Nullable properties/fields.

clrudolphi · 2024-07-10T15:29:47Z

@mpkorstanje @luke-hill Any chance to close this out today or tomorrow? I'm going on holiday until the 1st of August and would be good to finish this so that @luke-hill can proceed with his other refactoring.

luke-hill · 2024-07-11T13:30:49Z

@clrudolphi consider all my comments that have not been addressed to be "deferable" I don't want to hold anything up.

Just consider the actual dotnet stuff from Gaspar / Rien now. I can do some more fine tuning after your stuff is merged in a separate PR entirely to both this one and my companion one. Which would probably make things easier for you to review also.

mpkorstanje

Looks good!

@gasparnagy any remarks? If not, feel free to merge.

gasparnagy

Shit it 😀!

clrudolphi · 2024-07-12T10:00:25Z

Shit it 😀!

Funniest merge approval comment ever!

gasparnagy · 2024-07-12T11:26:43Z

😀 Autocorrect... But you know what I meant.

mpkorstanje · 2024-07-12T11:44:15Z

@gasparnagy have you made a release before?

gasparnagy · 2024-07-12T11:46:41Z

No, I don't think so.

luke-hill · 2024-08-08T12:17:52Z

Just posting here. Let's hold off releasing until I get my codegen stuff in also. As then it's a chunky enough release for a major.

mpkorstanje · 2024-08-08T12:57:15Z

No problem.

clrudolphi added 10 commits June 27, 2024 21:41

Initial Commit of using erb to generate dotnet

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

c5840f4

fixup syntax errors

b90e60e

fixup mistake with enums

4b9f301

added using statement for System.ComponentModel so that Description a…

24685ba

…ttributes will work.

added using statement for System.ComponentModel so that Description a…

ea833bf

…ttributes will work.

Merge branch 'RefactorDotnetCodeGen' of https://github.com/clrudolphi…

792de3e

…/messages into RefactorDotnetCodeGen

added using statement for System.Reflection to the enum generator

213fba6

regenerated the enums with the previous fix

8b652ea

Fixed enums by creating Extensions class for each enum

1521062

Fixup nullable value types,

7044cb3

Reverted to .NET Standard 2.0, Passing simple test.

clrudolphi marked this pull request as draft June 28, 2024 18:03

mpkorstanje marked this pull request as ready for review June 28, 2024 18:49

mpkorstanje marked this pull request as draft June 28, 2024 18:49

mpkorstanje requested changes Jun 28, 2024

View reviewed changes

mpkorstanje and others added 2 commits June 29, 2024 22:22

Setup Make, make build fail

3cdf56a

Moved Generated Files to the /generated folder. Added support classes…

e4aa561

… for serialization and TimeStamp handling. Added tests.

clrudolphi marked this pull request as ready for review June 30, 2024 01:20

mpkorstanje requested a review from gasparnagy June 30, 2024 07:54

mpkorstanje reviewed Jun 30, 2024

View reviewed changes

dotnet/Cucumber.Messages.Specs/ProtocolVersionTest.cs Outdated Show resolved Hide resolved

Modified github workflow test-dotnet.yml to invoke dotnet test during…

8e274b3

… testing step. (Updated minimum dotnet version from 5.x to 8.x) Restored default.mk in the dotnet folder (was mistakenly deleted in an earlier commit)

gasparnagy requested changes Jul 1, 2024

View reviewed changes

gasparnagy mentioned this pull request Jul 1, 2024

After adding SpecFlow.Plus.LivingDocPlugin nuget the build of the project fails reqnroll/Reqnroll#195

Closed

Revisions based upon PR comments. All resolved except for decisions o…

b99f54b

…n: a) whether to use long or int, b) whether to take a dependency on System.Text.Json package or find another option.

Downgraded dependency on System.Text.JSON to v6.0.0 per comments in t…

4cf2841

…he PR. Put a lock around the creation of the json serialization options singleton to address what appear to be threading issues while running tests during github CI.

gasparnagy reviewed Jul 2, 2024

View reviewed changes

dotnet/Cucumber.Messages/NdjsonSerializer.cs Outdated Show resolved Hide resolved

Revised JsonOptions initializer from a lock sequence to Lazy<T>

2e6bd7c

mpkorstanje reviewed Jul 5, 2024

View reviewed changes

dotnet/Cucumber.Messages/CucumberMessagEnumConverter.cs Outdated Show resolved Hide resolved

mpkorstanje reviewed Jul 5, 2024

View reviewed changes

mpkorstanje added 3 commits July 5, 2024 17:22

Remove unused default.mk

97f4da6

Remove unused update-versions script

e4925a2

Remove unused default.mk

6061002

clrudolphi added 2 commits July 5, 2024 17:15

Clean-ups from review comments.

579edce

Merge branch 'RefactorDotnetCodeGen' of https://github.com/clrudolphi…

8b5849f

…/messages into RefactorDotnetCodeGen

luke-hill reviewed Jul 9, 2024

View reviewed changes

gasparnagy reviewed Jul 9, 2024

View reviewed changes

dotnet/Cucumber.Messages/CucumberMessagEnumConverter.cs Outdated Show resolved Hide resolved

Changes per review comments

d605e39

Remove dependabot config

cbfd326

mpkorstanje approved these changes Jul 11, 2024

View reviewed changes

Merge branch 'main' into RefactorDotnetCodeGen

49b45ad

gasparnagy approved these changes Jul 12, 2024

View reviewed changes

gasparnagy merged commit 3f63d3b into cucumber:main Jul 12, 2024
26 checks passed

mpkorstanje mentioned this pull request Aug 8, 2024

Refactored so that the dependency on System.Text.Json is removed from Cucumber.Messages deployable. #241

Merged

7 tasks

clrudolphi deleted the RefactorDotnetCodeGen branch September 13, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Refactor dotnet code gen #233

Refactor dotnet code gen #233

clrudolphi commented Jun 28, 2024 •

edited

Loading

mpkorstanje left a comment •

edited

Loading

mpkorstanje Jun 28, 2024

clrudolphi Jun 28, 2024

clrudolphi Jun 30, 2024

mpkorstanje commented Jun 28, 2024

clrudolphi commented Jun 30, 2024

clrudolphi commented Jun 30, 2024

mpkorstanje Jun 30, 2024 •

edited

Loading

clrudolphi Jun 30, 2024

gasparnagy left a comment

luke-hill commented Jul 2, 2024

mpkorstanje commented Jul 2, 2024

mpkorstanje Jul 5, 2024

clrudolphi Jul 5, 2024

clrudolphi commented Jul 5, 2024

luke-hill left a comment

clrudolphi commented Jul 9, 2024

clrudolphi commented Jul 10, 2024

luke-hill commented Jul 11, 2024

mpkorstanje left a comment •

edited

Loading

gasparnagy left a comment

clrudolphi commented Jul 12, 2024

gasparnagy commented Jul 12, 2024

mpkorstanje commented Jul 12, 2024

gasparnagy commented Jul 12, 2024

luke-hill commented Aug 8, 2024

mpkorstanje commented Aug 8, 2024

Refactor dotnet code gen #233

Refactor dotnet code gen #233

Conversation

clrudolphi commented Jun 28, 2024 • edited Loading

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

mpkorstanje left a comment • edited Loading

Choose a reason for hiding this comment

mpkorstanje Jun 28, 2024

Choose a reason for hiding this comment

clrudolphi Jun 28, 2024

Choose a reason for hiding this comment

clrudolphi Jun 30, 2024

Choose a reason for hiding this comment

mpkorstanje commented Jun 28, 2024

clrudolphi commented Jun 30, 2024

clrudolphi commented Jun 30, 2024

mpkorstanje Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

clrudolphi Jun 30, 2024

Choose a reason for hiding this comment

gasparnagy left a comment

Choose a reason for hiding this comment

luke-hill commented Jul 2, 2024

mpkorstanje commented Jul 2, 2024

mpkorstanje Jul 5, 2024

Choose a reason for hiding this comment

clrudolphi Jul 5, 2024

Choose a reason for hiding this comment

clrudolphi commented Jul 5, 2024

luke-hill left a comment

Choose a reason for hiding this comment

clrudolphi commented Jul 9, 2024

clrudolphi commented Jul 10, 2024

luke-hill commented Jul 11, 2024

mpkorstanje left a comment • edited Loading

Choose a reason for hiding this comment

gasparnagy left a comment

Choose a reason for hiding this comment

clrudolphi commented Jul 12, 2024

gasparnagy commented Jul 12, 2024

mpkorstanje commented Jul 12, 2024

gasparnagy commented Jul 12, 2024

luke-hill commented Aug 8, 2024

mpkorstanje commented Aug 8, 2024

clrudolphi commented Jun 28, 2024 •

edited

Loading

mpkorstanje left a comment •

edited

Loading

mpkorstanje Jun 30, 2024 •

edited

Loading

mpkorstanje left a comment •

edited

Loading