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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/release-nuget.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ jobs:
environment: Release
steps:
- uses: actions/checkout@v4

- name: Setup .NET
uses: actions/setup-dotnet@v4
with:
dotnet-version: 6.0.x
dotnet-version: 8.0.x
Romfos marked this conversation as resolved.
Show resolved Hide resolved

- uses: cucumber/[email protected]
with:
nuget-api-key: ${{ secrets.NUGET_API_KEY }}
Expand Down
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]
### Fixed
- [Dotnet] Fixed code generation for types that accept List<T> as parameters. Constructors were not properly handling null input. ([#249](https://github.com/cucumber/messages/pull/249) [clrudolphi])

### Added
- [Dotnet] Run tests for .NET 8 + .NET Framework 4.6.2
- [Dotnet] Added editorconfig

### Changed
- [Go] Switch to Google's UUID module ([#251](https://github.com/cucumber/messages/pull/251))
- [Dotnet] Update target frameworks: .NET 8, .NET Standard 2.0
- [Dotnet] Adopt C# 12 syntax
- [Dotnet] Enable warnings as errors
- [Dotnet] Enable nulalble for all projects
Romfos marked this conversation as resolved.
Show resolved Hide resolved

## [26.0.0] - 2024-08-15
### Added
Expand Down
2 changes: 1 addition & 1 deletion codegen/generators/dotnet.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ module Generator
# Automatic Code generation overrides for the .NET programming language
class Dotnet < Base
def array_type_for(type_name)
"List<#{type_name}>"
"ImmutableArray<#{type_name}>"
end

def format_description(raw_description, indent_string: '')
Expand Down
102 changes: 15 additions & 87 deletions codegen/templates/dotnet.dotnet.erb
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
<%- @schemas.each do |key, schema| -%>
<%= class_name(key) %>.cs
using System;
using System.Collections.Generic;

// ------------------------------------------------------------------------------
// This code was generated based on the Cucumber JSON schema
// Changes to this file may cause incorrect behavior and will be lost if
// the code is regenerated.
// ------------------------------------------------------------------------------
using System.Collections.Immutable;

namespace Io.Cucumber.Messages.Types;

Expand All @@ -20,21 +19,20 @@ namespace Io.Cucumber.Messages.Types;
<%- end -%>
*/

public sealed class <%= class_name(key) %>
public sealed record <%= class_name(key) %>
{
<%- schema['properties'].each do |property_name, property|
required = (schema['required'] || []).index(property_name)
isValueType = type_for(class_name(key), property_name, property) == 'long' || type_for(class_name(key), property_name, property) == 'bool'
required = (schema['required'] || []).index(property_name)
-%>
<%- unless (property['description'] || []).empty? -%>
/**
<%= format_description(property['description'], indent_string: ' ') %>
*/
<%- end -%>
<%- if isValueType && !required -%>
public Nullable<<%= type_for(class_name(key), property_name, property) %>> <%= capitalize(property_name) %> { get; private set; }
<%- if !required -%>
public <%= type_for(class_name(key), property_name, property) %>? <%= capitalize(property_name) %> { get; }
<%- else -%>
public <%= type_for(class_name(key), property_name, property) %> <%= capitalize(property_name) %> { get; private set; }
public <%= type_for(class_name(key), property_name, property) %> <%= capitalize(property_name) %> { get; }
<%- end -%>
<%- end -%>

Expand All @@ -46,8 +44,8 @@ public sealed class <%= class_name(key) %>
return new <%= class_name(key) %>(
<%- schema['properties'].each.with_index(1) do |(property_name_2, _property_2), index| -%>
<%- if property_name_2 == property_name -%>
Require<<%= type_for(class_name(key), property_name, property) %>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null")<%= index < schema['properties'].length ? ',' : '' %>
<%- else -%>
<%= property_name %> ?? throw new ArgumentNullException("<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null")<%= index < schema['properties'].length ? ',' : '' %>
<%- else -%>
null<%= index < schema['properties'].length ? ',' : '' %>
<%- end -%>
<%- end -%>
Expand All @@ -58,96 +56,26 @@ public sealed class <%= class_name(key) %>

public <%= class_name(key) %>(
<%- schema['properties'].each.with_index(1) do |(property_name, property), index|
isValueType = type_for(class_name(key), property_name, property) == 'long' || type_for(class_name(key), property_name, property) == 'bool'
required = (schema['required'] || []).index(property_name)
-%>
<%- if isValueType && !required -%>
Nullable<<%= type_for(class_name(key), property_name, property) %>> <%= property_name %><%= index < schema['properties'].length ? ',' : ''%>
<%- if !required -%>
<%= type_for(class_name(key), property_name, property) %>? <%= property_name %><%= index < schema['properties'].length ? ',' : ''%>
<%- else -%>
<%= type_for(class_name(key), property_name, property) %> <%= property_name %><%= index < schema['properties'].length ? ',' : ''%>
<%- end -%>
<%- end -%>
)
{
<%- schema['properties'].each do |property_name, property|
required = (schema['required'] || []).index(property_name)
required = (schema['required'] || []).index(property_name)
isValueType = type_for(class_name(key), property_name, property) == 'long' || type_for(class_name(key), property_name, property) == 'bool' || property['enum'] != nil || property['type'] == 'array'
-%>
<%- if required -%>
RequireNonNull<<%= type_for(class_name(key), property_name, property) %>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");
<%- end -%>
<%- if property['items'] -%>
<%- if required -%>
this.<%= capitalize(property_name) %> = new <%= type_for(class_name(key), property_name, property) %>(<%= property_name %>);
<%- else -%>
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");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Romfos Romfos Sep 15, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

PR is updated

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@Romfos Romfos Sep 17, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

@Romfos Romfos Sep 21, 2024

Choose a reason for hiding this comment

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

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

<%- else -%>
this.<%= capitalize(property_name) %> = <%= property_name %>;
<%= capitalize(property_name) %> = <%= property_name %>;
<%- end -%>
<%- end -%>
}

public override bool Equals(Object o)
{
if (this == o) return true;
if (o == null || this.GetType() != o.GetType()) return false;
<%= class_name(key) %> that = (<%= class_name(key) %>) o;
return <%- schema['properties'].each.with_index(1) do |(property_name, _property), index| %>
<%- if (schema['required'] || []).index(property_name) -%>
<%= capitalize(property_name) %>.Equals(that.<%= capitalize(property_name) %>)<%= index < schema['properties'].length ? ' && ' : ';' -%>
<%- else -%>
Object.Equals(<%= capitalize(property_name) %>, that.<%= capitalize(property_name) %>)<%= index < schema['properties'].length ? ' && ' : ';' -%>
<%- end -%>
<% end %>
}

public override int GetHashCode()
{
int hash = 17;
<%- schema['properties'].each do |property_name, property|
required = (schema['required'] || []).index(property_name)
isValueType = type_for(class_name(key), property_name, property) == 'long' || type_for(class_name(key), property_name, property) == 'bool'
isEnum = property['enum']
-%>
<%- if isEnum -%>
hash = hash * 31 + <%= capitalize(property_name) %>.GetHashCode();
<%- elsif isValueType && !required -%>
if (<%= capitalize(property_name) -%>.HasValue)
hash = hash * 31 + <%= capitalize(property_name) %>.Value.GetHashCode();
<%- elsif isValueType -%>
hash = hash * 31 + <%= capitalize(property_name) %>.GetHashCode();
<%- else -%>
if (<%= capitalize(property_name) -%> != null)
hash = hash * 31 + <%= capitalize(property_name) %>.GetHashCode();
<%- end -%>
<%- end -%>
return hash;
}

public override string ToString()
{
return "<%= class_name(key) %>{" +
<%- schema['properties'].each_with_index do |(property_name, property), index|
required = (schema['required'] || []).index(property_name)
isValueType = type_for(class_name(key), property_name, property) == 'long' || type_for(class_name(key), property_name, property) == 'bool'
-%>
<%- if isValueType && !required -%>
(<%= capitalize(property_name) %>.HasValue ? "<%= index.zero? ? '' : ', '%><%= property_name %>=" + <%= capitalize(property_name) %>.Value : "") +
<%- else -%>
"<%= index.zero? ? '' : ', '%><%= property_name %>=" + <%= capitalize(property_name) %> +
<%- end -%>
<%- end -%>
'}';
}

private static T Require<T>(T property, string propertyName, string errorMessage)
{
RequireNonNull<T>(property, propertyName, errorMessage);
return property;
}
private static void RequireNonNull<T>(T property, string propertyName, string errorMessage)
{
if (property == null) throw new ArgumentNullException(propertyName, errorMessage);
}
}
<% end -%>
9 changes: 5 additions & 4 deletions codegen/templates/dotnet.enum.dotnet.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
<% @enums.each do |enum| -%>
<%= enum[:name] %>.cs
using System;
using System.ComponentModel;
using System.Reflection;

Expand All @@ -12,7 +11,8 @@ namespace Io.Cucumber.Messages.Types;
// the code is regenerated.
// ------------------------------------------------------------------------------

public enum <%= enum[:name] %> {
public enum <%= enum[:name] %>
{
<% enum[:values].each.with_index(1) do |value, index| -%>

[Description("<%= value%>")]
Expand All @@ -22,8 +22,9 @@ public enum <%= enum[:name] %> {

public static class <%= enum[:name] %>Extensions
{
public static string Value(this <%= enum[:name] %> v) {
var attribute = v.GetType().GetField(v.ToString()).GetCustomAttribute<DescriptionAttribute>();
public static string Value(this <%= enum[:name] %> v)
{
var attribute = v.GetType().GetField(v.ToString())?.GetCustomAttribute<DescriptionAttribute>();
return attribute == null ? v.ToString() : attribute.Description;
}
}
Expand Down
12 changes: 12 additions & 0 deletions dotnet/.editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
root=true

[*]
indent_style=space
end_of_line=crlf
charset=utf-8

[*.{csproj,props}]
indent_size=2

[*.cs]
indent_size=4
2 changes: 2 additions & 0 deletions dotnet/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
*.sln.docstates
*.ide

.vs/

# Build results

[Dd]ebug/
Expand Down
Binary file not shown.
93 changes: 42 additions & 51 deletions dotnet/Cucumber.Messages.Specs/BasicMessageSerializationTests.cs
Original file line number Diff line number Diff line change
@@ -1,69 +1,60 @@
using Xunit;
using System.IO;
using Io.Cucumber.Messages.Types;
using System.Text.Json;
using System.Collections.Generic;
using System.Reflection;
using System.Text.Json.Serialization;
using System;
using System.ComponentModel;
using Cucumber.Messages;
using System.Linq;

namespace Cucumber.Messages.Specs
{
public class BasicMessageSerializationTests
{
using Xunit;

[Fact]
public void SerializesAnEnvelopeToNDJSONCorrectly()
{
var stepDefinitionNDJSON = @"{""stepDefinition"":{""id"":""0"",""pattern"":{""source"":""I have {int} cukes in my belly"",""type"":""CUCUMBER_EXPRESSION""},""sourceReference"":{""location"":{""line"":3},""uri"":""samples/minimal/minimal.feature.ts""}}}
";
var sourceReference = new SourceReference("samples/minimal/minimal.feature.ts",
null, null, new Location(3, null));
var stepDefPattern = new StepDefinitionPattern("I have {int} cukes in my belly", StepDefinitionPatternType.CUCUMBER_EXPRESSION);
var stepDefinition = new StepDefinition("0", stepDefPattern, sourceReference);
var env = new Envelope(null, null, null, null, null, null, null, null, stepDefinition, null, null, null, null, null, null, null, null);
namespace Cucumber.Messages.Specs;

var serializedStepDefinition = NdjsonSerializer.Serialize(env);
var reconstructedStepDefinition = NdjsonSerializer.Deserialize(serializedStepDefinition);
public class BasicMessageSerializationTests
{

var expectedStepDefinition = NdjsonSerializer.Deserialize(stepDefinitionNDJSON);
Assert.Equal(expectedStepDefinition, reconstructedStepDefinition);
}
[Fact]
public void SerializesAnEnvelopeToNDJSONCorrectly()
{
var stepDefinitionNDJSON = @"{""stepDefinition"":{""id"":""0"",""pattern"":{""source"":""I have {int} cukes in my belly"",""type"":""CUCUMBER_EXPRESSION""},""sourceReference"":{""location"":{""line"":3},""uri"":""samples/minimal/minimal.feature.ts""}}}
";
var sourceReference = new SourceReference("samples/minimal/minimal.feature.ts",
null, null, new Location(3, null));
var stepDefPattern = new StepDefinitionPattern("I have {int} cukes in my belly", StepDefinitionPatternType.CUCUMBER_EXPRESSION);
var stepDefinition = new StepDefinition("0", stepDefPattern, sourceReference);
var env = new Envelope(null, null, null, null, null, null, null, null, stepDefinition, null, null, null, null, null, null, null, null);

var serializedStepDefinition = NdjsonSerializer.Serialize(env);
var reconstructedStepDefinition = NdjsonSerializer.Deserialize(serializedStepDefinition);

var expectedStepDefinition = NdjsonSerializer.Deserialize(stepDefinitionNDJSON);
Assert.Equal(expectedStepDefinition, reconstructedStepDefinition);
}

[Fact]
public void ProperlyDeserializesCollectionProperties() {
[Fact]
public void ProperlyDeserializesCollectionProperties()
{

// The following is from the CCK hooks sample
var json = @"{""id"":""33"",""pickleId"":""20"",""testSteps"":[{""hookId"":""0"",""id"":""29""},{""hookId"":""1"",""id"":""30""},{""id"":""31"",""pickleStepId"":""19"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""32""}]}";
// The following is from the CCK hooks sample
var json = @"{""id"":""33"",""pickleId"":""20"",""testSteps"":[{""hookId"":""0"",""id"":""29""},{""hookId"":""1"",""id"":""30""},{""id"":""31"",""pickleStepId"":""19"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""32""}]}";

// This test will pass if the deserializer does not throw an exception
var testCase = NdjsonSerializer.Deserialize<TestCase>(json);
Assert.Equal(4, testCase.TestSteps.Count);
// This test will pass if the deserializer does not throw an exception
var testCase = NdjsonSerializer.Deserialize<TestCase>(json);
Assert.Equal(4, testCase.TestSteps.Length);

var json2 = @"{""id"":""43"",""pickleId"":""24"",""testSteps"":[{""hookId"":""0"",""id"":""39""},{""hookId"":""1"",""id"":""40""},{""id"":""41"",""pickleStepId"":""23"",""stepDefinitionIds"":[],""stepMatchArgumentsLists"":[]},{""hookId"":""4"",""id"":""42""}]}";
var json2 = @"{""id"":""43"",""pickleId"":""24"",""testSteps"":[{""hookId"":""0"",""id"":""39""},{""hookId"":""1"",""id"":""40""},{""id"":""41"",""pickleStepId"":""23"",""stepDefinitionIds"":[],""stepMatchArgumentsLists"":[]},{""hookId"":""4"",""id"":""42""}]}";

// This test will pass if the deserializer does not throw an exception
var testCase2 = NdjsonSerializer.Deserialize<TestCase>(json2);
Assert.Equal(4, testCase2.TestSteps.Count);
// This test will pass if the deserializer does not throw an exception
var testCase2 = NdjsonSerializer.Deserialize<TestCase>(json2);
Assert.Equal(4, testCase2.TestSteps.Length);

var envText = @"{""testCase"":{""id"":""33"",""pickleId"":""20"",""testSteps"":[{""hookId"":""0"",""id"":""29""},{""hookId"":""1"",""id"":""30""},{""id"":""31"",""pickleStepId"":""19"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""32""}]}}
var envText = @"{""testCase"":{""id"":""33"",""pickleId"":""20"",""testSteps"":[{""hookId"":""0"",""id"":""29""},{""hookId"":""1"",""id"":""30""},{""id"":""31"",""pickleStepId"":""19"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""32""}]}}
{""testCase"":{""id"":""38"",""pickleId"":""22"",""testSteps"":[{""hookId"":""0"",""id"":""34""},{""hookId"":""1"",""id"":""35""},{""id"":""36"",""pickleStepId"":""21"",""stepDefinitionIds"":[""3""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""4"",""id"":""37""}]}}
{""testCase"":{""id"":""43"",""pickleId"":""24"",""testSteps"":[{""hookId"":""0"",""id"":""39""},{""hookId"":""1"",""id"":""40""},{""id"":""41"",""pickleStepId"":""23"",""stepDefinitionIds"":[],""stepMatchArgumentsLists"":[]},{""hookId"":""4"",""id"":""42""}]}}
{""testCase"":{""id"":""49"",""pickleId"":""26"",""testSteps"":[{""hookId"":""0"",""id"":""44""},{""hookId"":""1"",""id"":""45""},{""id"":""46"",""pickleStepId"":""25"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""5"",""id"":""47""},{""hookId"":""4"",""id"":""48""}]}}
{""testCase"":{""id"":""55"",""pickleId"":""28"",""testSteps"":[{""hookId"":""0"",""id"":""50""},{""hookId"":""1"",""id"":""51""},{""id"":""52"",""pickleStepId"":""27"",""stepDefinitionIds"":[""2""],""stepMatchArgumentsLists"":[{""stepMatchArguments"":[]}]},{""hookId"":""6"",""id"":""53""},{""hookId"":""4"",""id"":""54""}]}}";
var lines = envText.Replace("\r\n", "\n").Split("\n").ToList();
List<Envelope> envelopes = new List<Envelope>();
var lines = envText.Replace("\r\n", "\n").Split('\n').ToList();

List<Envelope> envelopes = new List<Envelope>();

foreach (var line in lines)
{
var envelope = NdjsonSerializer.Deserialize<Envelope>(line);
envelopes.Add(envelope);
}
Assert.Equal(5, envelopes.Count);
foreach (var line in lines)
{
var envelope = NdjsonSerializer.Deserialize<Envelope>(line);
envelopes.Add(envelope);
}
Assert.Equal(5, envelopes.Count);
}
}
Loading
Loading