Create Dart Implementation#431
Conversation
…mprove time conversion functions to truncate sub-microsecond precision. Add tests for error scenarios and precision handling.
…n JSON object structure and stream processing behavior.
There was a problem hiding this comment.
Heya, thanks for the PR!
I've given this a quick look for big problems. I didn't spot any, but I'll have to give it a more detailed look later.
Some general remarks:
-
I assume you're aware of cucumber/gherkin#30 and cucumber/gherkin#29.
-
We currently don't publish the Gherkin parser. I assume that is something you'd like to see happen in the future. That can be arranged though but might take some work.
-
I see that you've chosen to set default values for all required fields. Perhaps copied from the JavaScript implementation. We are trying to get rid of those for JavaScript (#285). It would make sense not copy that mistake into Dart. Ideally the constructor would throw an exception when trying to create message with missing required fields.
-
The generated messages sit in the same folder as the support code. This makes reviewing code quite annoying. And this may cause issues in the future if introduce a message that clashes with the support code. Can we separate these?
A few more detailed remarks below.
| // ignore_for_file: cast_nullable_to_non_nullable, public_member_api_docs | ||
| // ignore_for_file: sort_constructors_first | ||
| // ignore_for_file: unnecessary_null_in_if_null_operators | ||
| HEADER |
There was a problem hiding this comment.
For consistency, please move this to the template. That's where it is expected.
There was a problem hiding this comment.
Moved to where this is expected
| def class_name(ref) | ||
| base = File.basename(ref, '.schema.json') | ||
| return 'DurationMessage' if base == 'Duration' | ||
| return 'ExceptionMessage' if base == 'Exception' |
There was a problem hiding this comment.
If this is necessary, this could do with a comment to explain why these names are different. Though I assume Dart has some name spacing to prevent collisions in the first place, if so then using the message names would be preferable. People very familiar with messages, but not so familiar with Dart will occasionally have to make small changes to implementations that use Dart. This would likely trip them up.
There was a problem hiding this comment.
It collides with some foundational dart classes. I refactored to use the normal names & refactored code that depends on it with explicit references to the file the class was from. Ex:
import 'package:cucumber_messages/src/messages.dart' as messages;
/// Converts a Dart [Duration] to a Cucumber [messages.Duration].
///
/// The [duration] value is exported as whole seconds plus nanoseconds.
messages.Duration durationToDurationMessage(Duration duration) {
final micros = duration.inMicroseconds;
final seconds = micros ~/ Duration.microsecondsPerSecond;
final nanos = micros.remainder(Duration.microsecondsPerSecond) * 1000;
return messages.Duration(seconds: seconds, nanos: nanos);
}
| part of 'messages.dart'; | ||
|
|
||
| class DurationMessage { | ||
| final int seconds; |
There was a problem hiding this comment.
What is the range on integer values in dart?
There was a problem hiding this comment.
From first-party docs:
The default implementation of int is 64-bit two's complement integers with operations that wrap to that range on overflow.
Note: When compiling to JavaScript, integers are restricted to values that can be represented exactly by double-precision floating point values. The available integer values include all integers between -2^53 and 2^53, and some integers with larger magnitude. That includes some integers larger than 2^63. The behavior of the operators and methods in the int class therefore sometimes differs between the Dart VM and Dart code compiled to JavaScript. For example, the bitwise operators truncate their operands to 32-bit integers when compiled to JavaScript.
There was a problem hiding this comment.
So basically:
| Platform | Underlying Representation | Minimum Value | Maximum Value |
|---|---|---|---|
| Native | 64-bit signed integer | (-2^{63}) | (2^{63} - 1) |
| Web | 64-bit float (JS) | (-2^{53} + 1) (precise) | (2^{53} - 1) (precise) |
| } | ||
|
|
||
| /// Returns an [IdGenerator] that produces random RFC 4122 version 4 UUIDs. | ||
| IdGenerator uuidV4IdGenerator() { |
There was a problem hiding this comment.
I don't think it is a good idea to implement and maintain our own UUIDv4 generator. I also don't think it is a good idea to add a dependency on library that does this.
Ideally the IdGenerator is an interface that users of this library implement. For example by delegating to an existing UUIDv4 generator implementation that they pull in as a dependency.
There was a problem hiding this comment.
Agree, updated to only be an interface.
| Envelope parseEnvelopeJson( | ||
| String json, { | ||
| int? lineNumber, | ||
| bool includeLineInErrors = true, |
There was a problem hiding this comment.
Please default this to false. While not likely, it avoids accidental information disclosure type issues as well as logging of untrusted data.
There was a problem hiding this comment.
Please don't duplicate the CHANGELOG in the root of the project. This one will be overlooked if we make changes.
There was a problem hiding this comment.
Actually, re-added but had it point to the root changelog. pub.dev (official dart package distributor) has a scoring system for packages and having a changelog is one of the criteria. While it is not required, it can make the package not appear well-maintained & there is a GUI for the changelog on the site for potential adopters to reference.
If you would like, I can remove it again but felt like having it serve to only reference the root changelog was a fine solution to prevent the issue of having to maintain multiple versions.
There was a problem hiding this comment.
Also, looks like they require I include the current version number in the changelog or else there will be an error when validating publishing. I included that validation for publishing dry run in the CI to ensure it stays publishable but can understand it be annoying to have to bump the changelog value whenever the package version changes.
| matrix: | ||
| os: | ||
| - ubuntu-latest | ||
| dart: ["3.5.0", stable] |
There was a problem hiding this comment.
Please pin these to exact versions. This keeps CI stable.
|
|
||
| .generate-messages: $(schemas) ../codegen/codegen.rb ../codegen/templates/dart.dart.erb | ||
| ruby ../codegen/codegen.rb Generator::Dart dart.dart.erb > Generated.dart.tmp | ||
| ruby -e "input = File.readlines('Generated.dart.tmp', chomp: true); current = nil; lines = []; flush = lambda { next unless current; File.write(File.join('lib/src', current), lines.join(\"\\n\") + \"\\n\"); lines = [] }; input.each do |line| if line.end_with?('.g.dart') && !line.include?(' '); flush.call; current = line; else; lines << line; end; end; flush.call" |
There was a problem hiding this comment.
I assume this splits the generated file?
If so, please use the csplit solution used by other implementations.
Lines 21 to 25 in 521cb9d
There was a problem hiding this comment.
Updated to run using cpslit solution
There was a problem hiding this comment.
and yeah it does split the generated files
| with: | ||
| persist-credentials: false | ||
|
|
||
| - uses: dart-lang/setup-dart@c7a31f6a04bb9bf94f538fc551f7ef9e40223317 # v1.7.1 |
There was a problem hiding this comment.
Where did you get these hashes from? Have you used an LLM?
I also don't see any actions ran in https://github.com/Fried-man/messages/actions. Please this yourself before submitting.
There was a problem hiding this comment.
Yeah I used an LLM and it hallucinated. Updated to a real hash.
I tried to run it manually from the actions tab on my fork but didn't see an option to. Realized this morning that I could just open a PR on my fork and run the CI there. Here is a passing run from that.
| with: | ||
| persist-credentials: false | ||
|
|
||
| - uses: dart-lang/setup-dart@c7a31f6a04bb9bf94f538fc551f7ef9e40223317 # v1.7.1 |
|
|
||
| require: ## Check requirements for generation and checks | ||
| @ruby --version >/dev/null 2>&1 || (echo "ERROR: ruby is required."; exit 1) | ||
| @dart --version >/dev/null 2>&1 || (echo "ERROR: dart is required."; exit 1) |
There was a problem hiding this comment.
Dart should not be required to generate the code. I suspect that will go away once the bench mark tests are removed.
There was a problem hiding this comment.
Removed dart requirement. I was mainly using it to run dart format on the generated code. Not important to format generated code.
…es and updating code formatting for consistency. Adjust analysis options to exclude generated files and improve code generation process in Makefile.
…e error handling in NDJSON parsing.
…n generated files to prevent formatting issues.
…tors and implementing schema validation with SchemaViolationException. Update CHANGELOG to document these changes.
Hey! Thanks for the review
|
aedef0e to
f42f0ee
Compare
…ent for generated files
|
Alright, I think I'm done lol. Latest dart run passing on my fork can be found here. |
mpkorstanje
left a comment
There was a problem hiding this comment.
Great! I don't have time for another review pass today.
But I was reading https://pubrelease.onepub.dev/creating-a-release and I see that the pub_release command is rather involved and makes a lot of assumptions about the work flow.
Because we're releasing 10+ different languages we don't exactly have a standard process.
But it looks like this:
- Prepare the release by updating the version numbers and making other changes on a dev-machine.
- Tag these changes in Git.
- Push the changes and tags to GitHub.
- GitHub will trigger on the tags and start the release process for each action independently.
Can you tell me if the pub_release command is the only way to "publish your project to pub.dev"? We might have some experimenting to do to that get that work.
I believe the official publish path is dart pub publish. I don't believe it automatically creates tags or bumps the version. |
🤔 What's changed?
Adds an initial Dart implementation for Cucumber Messages
⚡️ What's your motivation?
There is currently no officially maintained Dart implementation of Cucumber Messages. I'd like to use a Dart implementation to build out Gherkin dart support.
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
If the API surface needs updates to better conform with the other language implementations.
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.