-
Notifications
You must be signed in to change notification settings - Fork 46
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
Create paced trains model and add post and delete endpoints #10842
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10842 +/- ##
==========================================
- Coverage 82.57% 81.63% -0.95%
==========================================
Files 1084 1098 +14
Lines 107404 110595 +3191
Branches 730 742 +12
==========================================
+ Hits 88687 90279 +1592
- Misses 18675 20274 +1599
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
49bacde
to
94fa0d3
Compare
94fa0d3
to
cdd4b0b
Compare
30bb380
to
cbfc4b6
Compare
a6dfdce
to
6fcf24c
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.
Cursory review.
I wonder if mod paced_train
shouldn't be included in mod timetable
since there's a bunch of visibility changes of existing definitions (that usually is a signal that something's wrong). (I know that we have DELETE /paced_trains
and not DELETE /timetable/paced_trains
, but URL path hierarchy and rust module trees are two different things that do not necessarily require correlation imo.)
Wdyt?
don't totally agree. We'd like to use paced trains in cases where the timetable is decorelated, as will perhaps be the case with train schedules. Maybe there won't be such a case, and if it's not the case I think it's a good idea to put paced train in timetable, but we'd have to put train schedule there too, wouldn't we? |
I'm not saying we should change the URL scheme, What I'm trying to say is that the module tree doesn't necessarily have to match our URLs paths. We're currently a bit forced to do that because of the |
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.
A first review, a few things are missing like indexes in the DB migration or the update of osrd_schemas
.
a82dd5a
to
2f81ec5
Compare
7507d4c
to
d589a6f
Compare
6b1b815
to
cd7dcb3
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.
Lgtm (not tested)
editoast/migrations/2025-02-13-115126_create_paced_trains/up.sql
Outdated
Show resolved
Hide resolved
python/railjson_generator/railjson_generator/schema/simulation/paced_train.py
Outdated
Show resolved
Hide resolved
cd7dcb3
to
fad67d2
Compare
883fc16
to
abd0bea
Compare
Signed-off-by: Youness CHRIFI ALAOUI <[email protected]>
abd0bea
to
e1d59ac
Compare
Close #10888