[.NET] Added .NET 8, NRT and more #253

Romfos · 2024-09-15T11:07:27Z

🤔 What's changed?

Update target frameworks: .NET 8, .NET Standard 2.0
Run tests for .NET 8 + .NET Framework
Adopt C# 12 syntax
Added editorconfig
Remove non exited files from solution
enable warnings as errors
Update .NET sdk for github actions

Template generator changes:

Enable NRT support
Switch from classes to records. Equals, GetHashCode, ToString will be autogenerated
Remove copy-paste helpers for every class like Require & RequireNonNull methods
Correct implementation for value types

⚡️ What's your motivation?

This PR is result of discussion here cucumber/gherkin#281

🏷️ What kind of change is this?

🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
💥 Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- 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.

Romfos · 2024-09-15T11:47:21Z

Important:
This PR is a "source breaking change".
Nullable annotations could lead to new warnings in code that depends on this library.
functionally library is the same

.github/workflows/release-nuget.yaml

clrudolphi · 2024-09-15T16:33:26Z

codegen/templates/dotnet.dotnet.erb

-        this.<%= capitalize(property_name) %> = <%= property_name %> == null ? null : new <%= type_for(class_name(key), property_name, property) %>(<%= property_name %>);
-        <%- end -%>
+      <%- if !isValueType && required -%>
+        <%= capitalize(property_name) %> = <%= property_name %> ?? throw new ArgumentNullException("<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");


The template was generating "record-like" classes with getters only; in a sense providing an 'immutable' data structure. This PR is adopting the use of records directly, making a stronger statement about immutability.
During the constructor, we previously copyied the contents of enumerable items by calling 'new List(argument) for those properties that were enumerable. This disconnected the Message instance from its source (if one list or the other got changed, the other would not).

This proposed template simply assigns the input argument to the generated property.

@gasparnagy How careful do we need to be about immutability and/or effects of change here?
We could use an ImmutableList here to be explicit about intent and strengthen the behavior, go back to previous behavior of simply copying the list content, or accept this behavior of accepting the List argument as-is.

ImmutableArray is desirable here, but i will require 1 extra dependency for .NET Standard tfm

I think regardless of what structure we use to store the "immutable" list, we should go back to the previous model of "copying" the items from the input structure, so the simple assignment is not enough.

As this is package is not really used yet, maybe we should be more future proof and go for the ImmutableList accepting the fact that this way we have to add a dependency to System.Collections.Immutable for the .NET Standard 2.0 target (in .NET 8 it is anyway included). So I would say, let's give it a try. If we see problems with this dependendency, we can still roll back.

PR is updated

Thx. As far as I see now it takes ImmutableArray as an input, so it will be the responsibility of the caller to ensure the immutability. I think this is good, but I wonder if we should generate an additional convenience overloads for the ctors that just take an IEnumerable<T> and build the ImmutableArray from it.
@clrudolphi Would creating an ImmutableArray for calling the ctors cause any issues for you?

I would prefer the constructor signature to accept the IEnumerable as a parameter, and have it create the ImutableArray property.

I disagree that we need to make a copy inside of constructor without any visible reason.
Maybe you have some code examples why it makes a problem and I can propose a some solution?

Let's clarify what we mean. Likewise, I do not think it necessary to create copies of the items within the source collections.
The design principle under consideration here is to provide a construction signature that is easy to use. The MS API Design Guidelines suggest that collection parameters be typed as interfaces and of a commonly used type, such as IEnumerable or IReadOnlyCollection.
As a convenience to users of the constructor, we can build an instance of an ImmutableArray from that provided IEnumberable argument and assign that to the property. Doing so doesn't copy the members of the collection (presuming T is a class, not a value type).
Since ImmutableArray is also IEnumerable, within the Messages library we can pass instances of ImmutableArray to constructors of other types (that accept IEnumberable) without needing an overloaded constructor.

If there is a concern about performance or memory overhead of creating a new instance of ImmutableArray out of an input IEnumberable, why not have the constructor test the run-time type of the argument, and if it is already an ImmutableArray, then simply assign it to the property; otherwise new up a new instance?

okay, then lets continue to use List copy as we have now

clrudolphi · 2024-09-15T19:03:34Z

Yeah. But Gaspar and I had previous conversations about this library and at the time had agreed to limit ourselves to .net 6 as the baseline. Let's let him weigh in tomorrow. Chris

________________________________ From: Romfos ***@***.***> Sent: Sunday, September 15, 2024 1:47:02 PM To: cucumber/messages ***@***.***> Cc: Chris Rudolphi ***@***.***>; Review requested ***@***.***> Subject: Re: [cucumber/messages] [.NET] Added .NET 8, NRT and more (PR #253) @Romfos commented on this pull request.

________________________________ In .github/workflows/release-nuget.yaml<#253 (comment)>:

- name: Setup .NET

uses: ***@***.*** with: - dotnet-version: 6.0.x + dotnet-version: 8.0.x this line change only sdk version for release github action. .NET standard 2.0 is still in tfm list — Reply to this email directly, view it on GitHub<#253 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAM7YMXVCF5N7MUY3WVLWZDZWXI2NAVCNFSM6AAAAABOHW4T6OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMBVGU2TGMRZGY>. You are receiving this because your review was requested.Message ID: ***@***.***>

gasparnagy

I think the overall goal and the concept of the PR is fine.
See my comment on the explicit listing of the .NET 8 target and also we should clean up the "immutable list" problem mentioned by @clrudolphi (see discussion there).

dotnet/Cucumber.Messages/Cucumber.Messages.csproj

CHANGELOG.md

clrudolphi · 2024-09-17T14:33:12Z

codegen/templates/dotnet.dotnet.erb

-        this.<%= capitalize(property_name) %> = <%= property_name %> == null ? null : new <%= type_for(class_name(key), property_name, property) %>(<%= property_name %>);
-        <%- end -%>
+      <%- if !isValueType && required -%>
+        <%= capitalize(property_name) %> = <%= property_name %> ?? throw new ArgumentNullException("<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");


I would prefer the constructor signature to accept the IEnumerable as a parameter, and have it create the ImutableArray property.

mpkorstanje · 2024-09-22T10:09:34Z

@Romfos did you close this by accident?

Romfos · 2024-09-22T11:27:50Z

@Romfos did you close this by accident?

no, accroding to comment #253 (comment) implementation will stay the same

mpkorstanje · 2024-09-22T13:37:40Z

Thanks for explaining!

clrudolphi · 2024-09-22T15:05:59Z

@Romfos Oh, I had missed that this had gotten closed. Even if we leave the collection class type unchanged, I would like to see the other changes proposed within this PR adopted.

[.NET] Upgrade Cucumber.Messages for dotnet

d2b13ac

Romfos force-pushed the main branch from d5c3d94 to fb0d45c Compare September 15, 2024 11:11

[.NET] Update code generator for dotent

9b9679a

Romfos force-pushed the main branch from fb0d45c to 9b9679a Compare September 15, 2024 11:14

Romfos marked this pull request as ready for review September 15, 2024 11:16

Romfos requested a review from clrudolphi September 15, 2024 11:16

Romfos mentioned this pull request Sep 15, 2024

[.NET] Add initial nullable reference types support cucumber/gherkin#281

Closed

7 tasks

Update generator formatting

99873a7

clrudolphi requested a review from gasparnagy September 15, 2024 16:16

clrudolphi reviewed Sep 15, 2024

View reviewed changes

gasparnagy requested changes Sep 17, 2024

View reviewed changes

dotnet/Cucumber.Messages/Cucumber.Messages.csproj Show resolved Hide resolved

Use ImmutableArray for array type in generator

6f9d7aa

Romfos requested review from clrudolphi and gasparnagy September 17, 2024 12:21

clrudolphi reviewed Sep 17, 2024

View reviewed changes

Fix changlog typo

dd83471

Romfos force-pushed the main branch from 24d35f5 to dd83471 Compare September 17, 2024 19:13

Romfos closed this Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[.NET] Added .NET 8, NRT and more #253

[.NET] Added .NET 8, NRT and more #253

Romfos commented Sep 15, 2024 •

edited

Loading

Romfos commented Sep 15, 2024 •

edited

Loading

clrudolphi Sep 15, 2024

Romfos Sep 15, 2024 •

edited

Loading

gasparnagy Sep 17, 2024

Romfos Sep 17, 2024

gasparnagy Sep 17, 2024

clrudolphi Sep 17, 2024

Romfos Sep 17, 2024 •

edited

Loading

clrudolphi Sep 18, 2024

Romfos Sep 21, 2024 •

edited

Loading

clrudolphi commented Sep 15, 2024 via email

gasparnagy left a comment

clrudolphi Sep 17, 2024

mpkorstanje commented Sep 22, 2024

Romfos commented Sep 22, 2024 •

edited

Loading

mpkorstanje commented Sep 22, 2024

clrudolphi commented Sep 22, 2024

[.NET] Added .NET 8, NRT and more #253

[.NET] Added .NET 8, NRT and more #253

Conversation

Romfos commented Sep 15, 2024 • edited Loading

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

Romfos commented Sep 15, 2024 • edited Loading

clrudolphi Sep 15, 2024

Choose a reason for hiding this comment

Romfos Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

gasparnagy Sep 17, 2024

Choose a reason for hiding this comment

Romfos Sep 17, 2024

Choose a reason for hiding this comment

gasparnagy Sep 17, 2024

Choose a reason for hiding this comment

clrudolphi Sep 17, 2024

Choose a reason for hiding this comment

Romfos Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

clrudolphi Sep 18, 2024

Choose a reason for hiding this comment

Romfos Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

clrudolphi commented Sep 15, 2024 via email

gasparnagy left a comment

Choose a reason for hiding this comment

clrudolphi Sep 17, 2024

Choose a reason for hiding this comment

mpkorstanje commented Sep 22, 2024

Romfos commented Sep 22, 2024 • edited Loading

mpkorstanje commented Sep 22, 2024

clrudolphi commented Sep 22, 2024

Romfos commented Sep 15, 2024 •

edited

Loading

Romfos commented Sep 15, 2024 •

edited

Loading

Romfos Sep 15, 2024 •

edited

Loading

Romfos Sep 17, 2024 •

edited

Loading

Romfos Sep 21, 2024 •

edited

Loading

Romfos commented Sep 22, 2024 •

edited

Loading