-
Notifications
You must be signed in to change notification settings - Fork 44
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
python: add tests for railjson_generator #6222
Conversation
2a08fab
to
56d7f70
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #6222 +/- ##
============================================
+ Coverage 26.92% 27.63% +0.71%
Complexity 2136 2136
============================================
Files 961 990 +29
Lines 124331 125811 +1480
Branches 2575 2575
============================================
+ Hits 33477 34771 +1294
- Misses 89364 89550 +186
Partials 1490 1490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
56d7f70
to
549e403
Compare
fcae1f8
to
3c9d27d
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.
Otherwise LGTM.
As an aside, we should probably change the docstrings to an imperative tense (https://peps.python.org/pep-0257/#one-line-docstrings)
python/railjson_generator/railjson_generator/external_generated_inputs.py
Outdated
Show resolved
Hide resolved
3c9d27d
to
a4c9844
Compare
Done, thanks. |
d8fdea9
to
1a39802
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.
Thanks, good PR !
I have only one thing to say, the ascii diagrams giving details about the infra used for the tests are good.
However, for a better understanding, I think we could represent a switch with 'o' or something to have a better understanding of the infra.
Especially in the case for routes tests, to avoid thinking detectors are link between track sections.
And I have no particuliar suggestion, but the difference between the diagram crossing and the diagram double slip switch is not explicit too.
For the routes tests, maybe we should add a comment to know what kind of route we are trying to create, but it's not mandatory, so as you wish !
1a39802
to
7cf0649
Compare
Thanks, I made a few adjustments. |
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 the CI part which looks good to me
7cf0649
to
2d801a5
Compare
fixes #6221