-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add messages implementation for python #165
base: main
Are you sure you want to change the base?
Conversation
18b7d09
to
6a26520
Compare
This address to #162 |
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 this doesn't follow the pattern used by the other language implementations in quite a few ways. Please follow up the directions from #162 around code generation.
I also don't understand the purpose of the samples directory.
|
You can use Pydantic if you can make it fit into the
Consider narrowing this down to a few representative examples. Currently it is hard to see the forest for the trees. |
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.
If you're going to copy lots of the cck it would be better to fetch the data using some form of call rather than C+P as this is currently being rapidly updated
ee63f2a
to
358b36b
Compare
What purpose do these tests serve? They'll be a hassle to update if/when the schema changes. |
Hi @elchupanebrej - Just checking in to see where you're up to with this. Is this something you're still working on? |
Hi @luke-hill, sorry for the long response, hadn't time to work on the project. I'll try to create another merge request that will conform to the building process. |
3256104
to
effdd2b
Compare
The PR was updated with Makefile. Model is stable, so generated code is totally same to version, which was generated at first try @mpkorstanje I kindly ask you to review the code and take a release part. I didn't get into all deps&relations between release tools. |
Left a few quick remarks, will have to take a deeper look later. |
@@ -0,0 +1,12 @@ | |||
{"meta":{"ci":{"buildNumber":"154666429","git":{"remote":"https://github.com/cucumber-ltd/shouty.rb.git","revision":"99684bcacf01d95875834d87903dcb072306c9ad"},"name":"GitHub Actions","url":"https://github.com/cucumber-ltd/shouty.rb/actions/runs/154666429"},"cpu":{"name":"x64"},"implementation":{"name":"fake-cucumber","version":"16.3.0"},"os":{"name":"darwin","version":"22.4.0"},"protocolVersion":"22.0.0","runtime":{"name":"node.js","version":"19.7.0"}}} |
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 this is a good process what you've done here. Just commenting for documentation.
I think as/when you have gotten this all working, it would be good to migrate this and others to the CCK proper. WDYT? (Maybe something for 2025?)
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.
migrate this and others to the CCK proper
It must work with CCK now in all possible cases. If it doesn't - let write tests & fix
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.
@elchupanebrej As a test I'm not happy with a "sample test". As said before this create a circular dependency between the code that generates the samples and messages.
Tests for messages can be limited to testing whether the code was generated and serialization works correctly. This is does not test those things specifically while still testing many other - less relevant things.
@luke-hill what exactly do you mean by "migrating this and others to the cck"?
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.
@mpkorstanje sorry for bothering you, it seems I can't catch a point:
Samples of messages in the CCK repository are stored as examples. Every tool that uses messages has to use them (at least serialize when some event is emitted, and deserialize when this message comes to some reporter). So I took the full suite of test data from the CCK repo and checked that the models generated were successfully parsed that messages into the model, and after that deserialized them to totally the same JSON. Could you please describe more precisely what kind of tests would be OK: would be enough if some model(for every kind of message) would be created, serialized and deserialized perfectly to the totally same model?
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.
The CCK uses the messages to generate the output of a canonical cucumber execution. For this is needs the messages. The value the CCK adds isn't that it generates a sample of each messages, but rather that the collection of messages as a whole. So it can for example express relationships between messages.
This dependency also means that it can't be used as test data in messages. That would result in a circular dependency.
Now for messages the exact testing strategy depends on the framework and language used.
For example for Javascript, the object and it's json representation are almost identical so there is little to test at all. And because the code is generated, it doesn't seem nesesary to test every message either.
So you can see we do a round trip test of one moderately complex message and not much more.
https://github.com/cucumber/messages/blob/main/javascript/test/messagesTest.ts
For Java serialization is more complicated. It does not have a concept of undefined
. So we got tests to check for that.
Now I don't know enough about Python to tell you exactly what to test. I can't tell you about pitfalls I don't know about. But I imagine if third party code generator is used, a simple round trip should be enough.
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.
To add as well. For the ruby implementation we don't even do something that complex. All we do is "open" the message (Which is an Envelope
class), then reclose it again. And check that in doing so we don't change anything. https://github.com/cucumber/messages/blob/main/ruby/spec/cucumber/messages/acceptance_spec.rb
If you're finding that you're going down the rabbit hole of doing lots of testing here, then something is likely going wrong. Messages are the building block that everything else is based off, not the other way around.
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.
Copied ruby approach
ae519d7
to
99e72d6
Compare
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.
There seems to have been a misunderstanding.
So just to clarify.
Either:
- Source is generated by the Ruby codegen script
- Generated source is checked in
Or:
- Source is generated by the python build process.
- Generated source is not checked in.
- Make targets print a message that code code gen is handled by Python.
Which option are you going for now?
@@ -0,0 +1,12 @@ | |||
{"meta":{"ci":{"buildNumber":"154666429","git":{"remote":"https://github.com/cucumber-ltd/shouty.rb.git","revision":"99684bcacf01d95875834d87903dcb072306c9ad"},"name":"GitHub Actions","url":"https://github.com/cucumber-ltd/shouty.rb/actions/runs/154666429"},"cpu":{"name":"x64"},"implementation":{"name":"fake-cucumber","version":"16.3.0"},"os":{"name":"darwin","version":"22.4.0"},"protocolVersion":"22.0.0","runtime":{"name":"node.js","version":"19.7.0"}}} |
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.
@elchupanebrej As a test I'm not happy with a "sample test". As said before this create a circular dependency between the code that generates the samples and messages.
Tests for messages can be limited to testing whether the code was generated and serialization works correctly. This is does not test those things specifically while still testing many other - less relevant things.
@luke-hill what exactly do you mean by "migrating this and others to the cck"?
python/src/messages.py
Outdated
@@ -0,0 +1,3 @@ | |||
from _messages import * | |||
|
|||
ExpressionType = Type1 |
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 don't understand what this file does. Can you explain?
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 have two entities in the original model named Type (design bug from my perspective). This module is a simple adapter, so the end user will import Type and ExpressionType but not Type and Type1. In the serialized model they both are named Type as it was in the original model
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.
Would it be possible to fix that in the code generator instead?
And if it is not possible, an explanatory comment would be useful.
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
python/pyproject.toml
Outdated
] | ||
dependencies = [ | ||
"importlib_resources", | ||
"pydantic>=2.0.3" |
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.
Is it really necessary to use and add pydantic as a dependency ?
Many people are still on pydantic v1, and this would require pytest-bdd users to upgrade to pydantic v2 since pytest-bdd will soon depend on gherkin
Arenβt stdlib dataclasses enough?
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.
importlib_resources is also, from what I can see, only used for tests which I'm not sure is needed either
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.
@youtux Yes, this is technically possible, but such realization will be dependent on some library like https://github.com/lidatong/dataclasses-json (the best option for now), which are not as good supported as pydantic
From another perspective - testing utilities are selected at the start of a project, so if the messages package will be used somewhere - it most probably would be dependent on the new version of Pydantic
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.
but there are many projects using pytest-bdd for years, and this would be an issue.
We can do without pydantic in a very simple way. We can use data classes, then when we need to serialise to json we call asdict(model)
. If we need custom encoders (e.g. for date times) we can implement a simple JsONEncoder and pass that to json.dumps(asdict(model), encoder=β¦)
.
Or also just implement custom serialiser for each object
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.
in this case, we have to implement dict_factory for dataclass.asdict, which will have to take in count Enums, or there would be an issue with serialization to JSON. And deserialisation to the dataclass also will be an issue (Enums again)
And pydantic covers both of this issues
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 really think we should not bring in a big dependency like pydantic here, especially since it has made a big API change in v2, and I can see it make it difficult for users to adopt this library if it conflicts with their pydantic v1 requirement.
What's the use of pydantic here? I don't see it being used for serialisation / deserialisation here.
What's the API of this library going to look like?
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.
Messages library is hardly used for serialization/deserialization, for example:
- Test runner must produce messages in the ndjson format, so it uses model of "messages" lib to represent outcomes, messages lib serializes and validates against Json schema (non-directly).
- Test reporter consumes ndjson stream of messages and uses "messages" library to deserialize inputs and validate them.
So "messages" lib is a bridge between test runner and test reporter (potentially from different languages ecosystems)
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.
ok, but how is the API of this lib supposed to look like?
from cucumber_messages import ???
???
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.
@youtux , please check python/tests/test_model_load.py test in this PR (I'll rework tests later).
For example reporting in the pytest-bdd-ng uses this particular model:
https://github.com/elchupanebrej/pytest-bdd-ng/blob/default/src/pytest_bdd/message_plugin.py
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.
Exrta dependencies were declined
Thanks for great review, return later this week and will update all things accordingly π |
c127cc1
to
f6ecf72
Compare
88ceac1
to
a4f4596
Compare
c3e24cb
to
0bd3ed1
Compare
55c3a88
to
575bfbb
Compare
This would use lidatong/dataclasses-json#442 in future |
7821dbc
to
b818731
Compare
@mpkorstanje @luke-hill @jsa34 @youtux
|
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've reviewed 2/3rds of this PR so far and only got a couple of questions. So I'll pass that in for now. The other items are a bit more complex and I'll review at a later date (Might be after xmas now).
@@ -0,0 +1,68 @@ | |||
# This code was generated using the code generator from cucumber-messages. |
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.
Whilst it's not mandatory usually we split the two templates up into an enum template and a non-enum (Standard class), template.
Usually this is because the enums generate just some simple readers for each constant whereas the class usually has some primitive behaviour (like serialization / deserialization).
An example of the complexity difference in ruby would be these two
ENUM: Just has constant definitions https://github.com/cucumber/messages/blob/main/ruby/lib/cucumber/messages/attachment_content_encoding.rb
Class: Has some basic readers/serialization logic (Note the attr_readers and the .from_h method https://github.com/cucumber/messages/blob/main/ruby/lib/cucumber/messages/attachment.rb
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.
The template is really simple, and all serialization/deserialization is done externally. Actually https://pypi.org/project/marshmallow-dataclass/ could be used instead of proposed serialization/deserialization approach
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.
Whilst the template is simple for all of the generators, we still partition them. Again it would be best if we keep things structured in the same way imo because then we can clearly see what moving parts support which portion of the codebase
e926466
to
fe353a2
Compare
[project.entry-points] | ||
|
||
[project.optional-dependencies] | ||
test = [ | ||
# local | ||
"cucumber-messages[test-coverage]", | ||
# external; Must be in sync with [tool.tox] | ||
"mypy", | ||
"pre-commit", | ||
"tox>=4.2" | ||
] | ||
test-coverage = [ | ||
"coverage", | ||
"GitPython", | ||
"packaging", | ||
"pytest" | ||
] | ||
|
||
[project.scripts] |
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.
project.entry_points
or project.scripts
needed or intended to be populated?
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.
Reviewed and signed off 17/21 files, still got a few things to query
uses: actions/setup-python@v5 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Generate code |
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 shouldn't be needed as this should be part of the regular code generation and checkin process
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.
It's needed for test execution later: We have to know if the model is fine for serialization/deserialization
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.
Sorry I think you're misunderstanding. The code "itself" is checked in (See this PR below for the code _messages.py
I think is the single file with all messages.
As part of your regular development flow you can develop new models and/or new facilities for enum's e.t.c. - Then once you run make generate
it will generate all your new code. This is the same flow as every other language
This code (Running make generate
), would therefore "re"generate the code on each and every PR which isn't something we would want to do, as it would create a rather large overhead. So you should just run make generate
whenever you want to update your messages
@@ -0,0 +1,68 @@ | |||
# This code was generated using the code generator from cucumber-messages. |
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.
Whilst the template is simple for all of the generators, we still partition them. Again it would be best if we keep things structured in the same way imo because then we can clearly see what moving parts support which portion of the codebase
Suggested file name of the attachment. (Provided by the user as an argument to `attach`) | ||
""" | ||
file_name: Optional[str] = None | ||
source: Optional["Source"] = None |
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.
[nit] we can use Source
instead of "Source"
, since we use from __future__ import annotations
""" | ||
The location of the `Background` keyword | ||
""" |
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.
[nit] can we inline this (and all the other similar ones?
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.
actually, we should use comments, see https://github.com/cucumber/messages/pull/165/files#r1912240401
""" | ||
* | ||
A comment in a Gherkin document | ||
""" | ||
""" | ||
The location of the 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.
this will cause Comment.__doc__
to include the text "The location of the comment". We should better use comments (e.g. # The location of the comment
)instead of strings to document attributes
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.
actually, I was wrong on this, it seems to work fine:
>>> class A:
... """docstring of A"""
... """docstring of foo"""
... foo: str
...
>>> A.__doc__
'docstring of A'
Although it looks like the correct way is to put the docstring after the attribute declaration: https://stackoverflow.com/a/69754746/714760
I must have missed something. Why are we not using the code generator from https://github.com/koxudaxi/datamodel-code-generator anymore? It would look more maintainable to me to use that one, as supporting the current proposed solution requires people that both both Ruby and Python, rather than just Python. |
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.
There's a few other minor things, but it'll be easier for me to just run rubocop or something on it afterwards as it's just syntax-y stuff.
I've ignored the generated files and the remaining 3 files to sign off have review stuff, so I've finished on this now. Great stuff.
|
||
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: |
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.
matrix: | |
matrix: | |
include: | |
# Test latest python on windows / macos | |
- { os: 'windows-latest', python-version: '3.13' } | |
- { os: 'macos-latest', python-version: '3.13' } | |
os: ['ubuntu-latest] | |
python-version: ['3.9',, '3.10', '3.11', '3.12', '3.13', 'pypy3.9', 'pypy3.10'] |
else | ||
"list[#{inner_type}]" | ||
end | ||
elsif type =~ /^[A-Z]/ |
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.
elsif type =~ /^[A-Z]/ | |
elsif type.match?(/\A[A-Z]/) |
type = type_for(parent_type_name, property_name, property) | ||
|
||
if type.start_with?('list[') | ||
list_type = type.match(/list\[(.*?)\]/) |
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.
Whilst this isn't wrong / buggy. Is there anything slightly more nuanced we can use for the inner capture. Maybe (\w+)
?
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.
Also you can optimise this as you're only using the regexp matcher briefly
type[/list\[(.*?)\]/]
This will not store it in a variable, but it will store it in Regexp.last_match -> The index you want is 1
so then in the statements below you can call Regexp.last_match(1)
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.
Isn't there a better way rather than doing regexp match? I don't know ruby idioms, but in python I would return a boxed object that wraps the subtype, like ArrayMarker(foo)
, and do an isinstwnce check here.
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 am not sure what you are trying to do here as it looks quite complicated.
Basically storing any regex details inside a #match
object is the heaviest footprint. And storing things briefly using Regexp.last_match is a "tiny" bit lighter, and Ideally just using #match?
is the best because this doesn't store anything
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.
If it's easier and you want me to tackle this as a follow-up can you make an issue and link to this given the size of this PR is quite large and first iteration is always the hardest one.
if type.start_with?('list[') | ||
list_type = type.match(/list\[(.*?)\]/) | ||
inner_type = list_type[1] | ||
if inner_type =~ /^[A-Z]/ |
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.
use .match? here too.
π€ What's changed?
Add python implementation
π·οΈ What kind of change is this?
π Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.