Skip to content
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

editoast: adapt conflicts endpoint to paced trains #10685

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

younesschrifi
Copy link
Contributor

Close #10684

@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Feb 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (8aaefd4) to head (35fb47b).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
...nt/src/applications/stdcm/utils/formatConflicts.ts 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10685      +/-   ##
==========================================
- Coverage   81.86%   81.86%   -0.01%     
==========================================
  Files        1082     1082              
  Lines      107119   107123       +4     
  Branches      721      721              
==========================================
+ Hits        87694    87696       +2     
- Misses      19384    19386       +2     
  Partials       41       41              
Flag Coverage Δ
editoast 74.32% <100.00%> (-0.01%) ⬇️
front 89.40% <80.00%> (ø)
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 87.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch from 7248a2b to 3582abe Compare February 11, 2025 11:54
@github-actions github-actions bot added the area:integration-tests Work on Integration test, by nature related to different services label Feb 11, 2025
@younesschrifi younesschrifi marked this pull request as ready for review February 11, 2025 11:56
@younesschrifi younesschrifi requested review from a team as code owners February 11, 2025 11:56
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch from 3582abe to b6dcee7 Compare February 12, 2025 06:36
Copy link
Contributor

@shenriotpro shenriotpro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for tests/

@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch 4 times, most recently from 29dfb51 to 9f84231 Compare February 13, 2025 09:19
@younesschrifi younesschrifi requested a review from a team as a code owner February 13, 2025 09:19
@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch 3 times, most recently from 59e63cb to 1ca3e36 Compare February 13, 2025 09:53
@younesschrifi younesschrifi requested a review from a team as a code owner February 13, 2025 09:53
@younesschrifi younesschrifi requested a review from eckter February 13, 2025 09:53
@github-actions github-actions bot added the area:core Work on Core Service label Feb 13, 2025
@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch from 1ca3e36 to 67cb12a Compare February 13, 2025 09:59
Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For core: seems like a simple renaming, I'm not checking further (I trust that this matches the new names used in the payload)

Copy link
Contributor

@Erashin Erashin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that this shouldn't be merged, I trust everyone else's opinion on the matter (especially if you know the subject). But from the pov of someone who has no idea what's going on, I clearly don't have enough info on this pr: i don't know what a paced train is, the commit has no additional info, description same, linked issue same, and no additional linked issue for additional info.
I'm guessing this is a first PR to pave the way for the following paced train conflict logic that will come in another PR. But yeah, I wouldn't be against having a bit more info somewhere for this sort of PR :)
As for the core part, lgtm, the rename works, and the added field isn't used yet anyhow.

@emersion
Copy link
Member

Yeah, agreed - I kind of realize we should've written down a design doc on the website for paced trains…

@Khoyo
Copy link
Contributor

Khoyo commented Feb 13, 2025

There's a confidential issue (https://github.com/osrd-project/osrd-confidential/issues/779). It should be cleaned up and put as a real osrd issue, and ideally we should have a design document on the website indeed.

@younesschrifi
Copy link
Contributor Author

younesschrifi commented Feb 13, 2025

I agree with everyone and I apologize for the lack of information.
I added a description in the issue.
I already made a PR in osrd_website (CF Timetable V2) to describe the paced train model and I'm going to add more information on it.
Thanks for the reviews.

@eckter @emersion @Erashin @Khoyo

@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch from 67cb12a to 427c61b Compare February 13, 2025 11:40
@younesschrifi younesschrifi force-pushed the yci/adapt-conflict-endpoint branch from 427c61b to 35fb47b Compare February 13, 2025 11:40
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm (for front part) and tested :)

@younesschrifi younesschrifi added this pull request to the merge queue Feb 13, 2025
@younesschrifi younesschrifi removed this pull request from the merge queue due to a manual request Feb 13, 2025
@younesschrifi younesschrifi added this pull request to the merge queue Feb 13, 2025
Merged via the queue into dev with commit 9badbe7 Feb 13, 2025
27 checks passed
@younesschrifi younesschrifi deleted the yci/adapt-conflict-endpoint branch February 13, 2025 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core Work on Core Service area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules area:integration-tests Work on Integration test, by nature related to different services
Projects
None yet
Development

Successfully merging this pull request may close these issues.