-
-
Notifications
You must be signed in to change notification settings - Fork 10
Add Dotnet Implementation #219
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
base: main
Are you sure you want to change the base?
Conversation
Fixed a problem when the input tag text is null.
gasparnagy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super. Looks good, I have made a few smaller comments.
| /// <exception cref="Exception">Always thrown to indicate a syntax error.</exception> | ||
| private void ThrowSyntaxError(string message) | ||
| { | ||
| throw new Exception($"Tag expression \"{_text}\" could not be parsed because of syntax error: {message}."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make a specific exception type for tag expression errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I had meant to and forgotten.
| </ItemGroup> | ||
|
|
||
| <PropertyGroup Label="Package Properties"> | ||
| <Product>TagExpressions</Product> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider calling this package and the namespaces to "Cucumber.TagExpressions" instead. The "TagExpressions" term is too generic (like we call the cucumber expressions package "Cucumber.CucumberExpressions", although there we have missed the opportunity to have a matching namespace).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
|
|
||
| internal class TagToken | ||
| { | ||
| public TagTokenType Type { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From other similar project experience, I have found it very useful to store the position of the token value as well withing the original expression. (I.e. the character index where this particular token has been found). This can be used to provide a better error message in case the error is related to a token. Like:
Invalid operation "and": "@foo and and @bar"
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will enhance the token to include the position, and include the Token object as a property of the custom exception type.
However, regarding including positioning information in the exception message: doing so would break the expectations set in the poly conformance tests as there is a specific content and format expected for each type of parsing error. (See cucumber/tag-expressions/testdata/errors.yml).
Would you rather that we:
- retain exact compatibility with the existing testsuite
- enhance the test suite to include positional information in error messages (and expect the other language implementations to match)
- diverge (and exclude the existing errors.yml tests from the dotnet implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now we can just retain the compatibility, but just have the information in the exception as you suggested, so if someone wants to have a better visualization for the error, they can do it.
| --> | ||
| <TestingPlatformShowTestsFailure>true</TestingPlatformShowTestsFailure> | ||
| <SignAssembly>True</SignAssembly> | ||
| <AssemblyOriginatorKeyFile>TagExpressionsTest.snk</AssemblyOriginatorKeyFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the TagExpressions.snk from the folder above (../TagExpressions.snk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
| using System; | ||
| using TagExpressions; | ||
|
|
||
| namespace TagExpressionsTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to file-scoped namespace declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
| using System.Text; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace QueryTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong namespace, use file-scoped namespace declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
| <PropertyGroup> | ||
| <LangVersion>13</LangVersion> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <TreatWarningsAsErrors>true</TreatWarningsAsErrors> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add <Nullable>enable</Nullable> here and handle nullable details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
dotnet/README.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| [](https://search.maven.org/search?q=g:%22io.cucumber%22%20AND%20a:%22tag-expressions%22) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be replaced to a NuGet badge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adopted
luke-hill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got to 15/27 items. Mainly non-poly stuff. Will get to others alter
| @@ -0,0 +1,3 @@ | |||
| using System.Runtime.CompilerServices; | |||
|
|
|||
| [assembly: InternalsVisibleTo("TagExpressionsTest, PublicKey=0024000004800000940000000602000000240000525341310004000001000100bd9b4db256712217f4906b5eb0113e633e319ef574450f83cb837097cac352cfac3a3d3e652c74e2f19e4af0464877e171602bb254fba62c9023894123393ca270ac5c1c01e99e233d0ccdb75f1ebf9b18b66f1618c1d9904658bc7e0a836c5488fb6e54845d9daabbf37594f54bc58f9136fd66cdad9b2761e8710956b2f1ce")] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a weird test? Not quite sure what it's doing. Just a comment to check if this is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement enables that the "TagExpressionsTest" project should have visibility on those stuff that are declared as "internal". This is a common practice in .NET.
Changed Namespaces. Added custom exception type. Include position information in Token. Corrected namespaces in Test project. Nullable by default. Readme.md now includes a nuget badge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a .NET implementation of tag expressions for Cucumber, enabling parsing and evaluation of logical tag expressions in .NET projects. The implementation follows the existing tag-expressions specification and includes comprehensive test coverage using YAML-based test data.
- Implements a complete tag expression parser with lexer, parser, and AST nodes
- Adds NuGet package configuration for distribution
- Includes extensive test suite with unit tests and YAML-based acceptance tests
Reviewed Changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/TagExpressions/TagExpressionParser.cs | Core parser implementing recursive descent parsing for tag expressions |
| dotnet/TagExpressions/TagLexer.cs | Lexer for tokenizing tag expression strings |
| dotnet/TagExpressions/TagExpression.cs | AST node classes representing parsed expressions |
| dotnet/TagExpressions/TagToken.cs | Token types and token class for lexical analysis |
| dotnet/TagExpressions/ITagExpression.cs | Interface defining the tag expression contract |
| dotnet/TagExpressionsTest/ParsingTest.cs | YAML-based tests for parsing behavior |
| dotnet/TagExpressionsTest/EvaluationsTest.cs | YAML-based tests for expression evaluation |
| dotnet/TagExpressionsTest/ErrorsTest.cs | YAML-based tests for error handling |
| dotnet/TagExpressionsTest/TagLexerTests.cs | Unit tests for the lexer |
| dotnet/TagExpressionsTest/TestFolderHelper.cs | Helper utility to locate test data files |
| dotnet/TagExpressions/Cucumber.TagExpressions.csproj | NuGet package configuration |
| .github/workflows/test-dotnet.yml | CI workflow for .NET testing |
| .github/workflows/release-nuget.yaml | Release workflow for NuGet publishing |
Comments suppressed due to low confidence (1)
dotnet/TagExpressionsTest/TagLexerTests.cs:94
- Generic catch clause.
catch (Exception ex)
{
StringAssert.Contains(ex.Message, "Illegal escape");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@clrudolphi please do take these into account: #222, #221. |
…ken ToString() should be empty string rather than "true").
@mpkorstanje This PR now matches the changes made in #221 and #222. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance it looks like you've made some implementation parts public. To make it easier to evolve the implementation - not that I expect much of it- it is generally nicer if the internal are kept internal. But I'm not a .Net expert so not 100% sure if that is actually the case.
Nitpicks aside, LGTM.
Feel free to merge. When you're happy.
| /// <summary> | ||
| /// Defines a parser for tag expressions. | ||
| /// </summary> | ||
| public interface ITagExpressionParser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity. Why an interface for the parser? There is only one concrete type and I don't imagine we'll ever have a second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the benefit of Reqnroll. It will have a Tag Expression parser implementation that wraps this one. See below for explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. But that means ITagExpressionParser can live with RnR right? You'd have a Cucumber implementation and a RnR implementation.
| /// <summary> | ||
| /// Provides an abstract base for tag expressions that can be evaluated against a set of input tags. | ||
| /// </summary> | ||
| public abstract class TagExpression : ITagExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this and other classes in this file be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need them to be public for Reqnroll.
The tag-expression library itself is not opiniated on whether literals in expressions must or must not always have the @ prefix.
Reqnroll is liberal in that respect. If a tag expression is given without, then Reqnroll rewrites the expression such that the tag literals in the expression are given the @ prefix.
In order to rewrite the expression, Reqnroll needs to be able to instantiate new instances of each of the TagExpression types.
public class ReqnrollTagExpressionParser(ITagExpressionParser tagExpressionParser) : ITagExpressionParser
{
public ITagExpression Parse(string tagExpression)
{
return Rewrite(tagExpressionParser.Parse(tagExpression));
}
private ITagExpression Rewrite(ITagExpression expression)
{
return expression switch
{
NullExpression nullExpression => nullExpression,
NotNode notNode => new NotNode(Rewrite(notNode.Operand)),
BinaryOpNode binaryOpNode => new BinaryOpNode(binaryOpNode.Op, Rewrite(binaryOpNode.Left), Rewrite(binaryOpNode.Right)),
LiteralNode literalNode => new LiteralNode(PrefixLiteral(literalNode.Name)),
_ => throw new NotSupportedException($"Unsupported tag expression type: {expression.GetType().FullName}"),
};
}
private string PrefixLiteral(string name)
{
if (name.IsNullOrEmpty() )
return name;
if (name.StartsWith("@"))
return name;
return "@" + name;
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with these classes being public, but then please do ensure they're not extensible, ect.
Co-authored-by: M.P. Korstanje <[email protected]>
Thanks for looking it over. |
On short time frames anything works. Ideally I'd get you onboarded with https://github.com/cucumber/polyglot-release then you can release it yourself. |
🤔 What's changed?
Add a .NET implementation.
This uses a hand-written parser/evaluator.
Passes tests equivalent to those in the Java implementation using the same testdata.
⚡️ What's your motivation?
Eventual incorporation into Reqnroll.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
Open to any feedback.
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.