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.

Sign up for GitHub

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

Jump to bottom

editoast: refacto path item location to add track and label information #9800

Merged
merged 2 commits into from
Nov 28, 2024
Merged

editoast: refacto path item location to add track and label information #9800

merged 2 commits into from
Nov 28, 2024

Conversation

younesschrifi
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024 •
edited
Loading

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

Codecov Report

Attention: Patch coverage is 91.96429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 38.19%. Comparing base (57c96f7) to head (e126d29).
Report is 93 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/path/path_item_cache.rs 89.28% 9 Missing ⚠️

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

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #9800      +/-   ##
==========================================
+ Coverage   37.82%   38.19%   +0.37%     
==========================================
  Files         994      998       +4     
  Lines       91126    92204    +1078     
  Branches     1176     1192      +16     
==========================================
+ Hits        34466    35220     +754     
- Misses      56206    56528     +322     
- Partials      454      456       +2
Flag Coverage Δ
editoast 73.39% <91.96%> (+0.38%) ⬆️
front 20.15% <ø> (+0.04%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 87.00% <ø> (+0.25%) ⬆️

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/refacto-path-item-location branch from 9ba7f2c to 239687f Compare November 24, 2024 22:08
@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Nov 24, 2024
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch 3 times, most recently from a4ea449 to 41128bd Compare November 25, 2024 09:10
@younesschrifi younesschrifi marked this pull request as ready for review November 25, 2024 09:14
@younesschrifi younesschrifi requested a review from a team as a code owner November 25, 2024 09:14
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch from 41128bd to d0ed7ef Compare November 25, 2024 09:45
woshilapin
woshilapin requested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

I'm missing a bit of context to understand exactly what is going on on the second commit. More generally, it would be nice to fill up the description of the PR?

Apart from that, thanks for the work, nice and easy to review. I have however a comment questionning the implementation. Leaving a "Request changes" until we know more.

editoast/editoast_schemas/src/train_schedule/path_item.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch 4 times, most recently from 47e283c to 78a0f76 Compare November 26, 2024 08:15
woshilapin
woshilapin approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Thanks for the modification of the implementation with TrackReference. I've made a few more comments, but nothing blocking now 🎉

editoast/editoast_schemas/src/train_schedule/path_item.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
flomonster
flomonster requested changes Nov 26, 2024
View reviewed changes
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.

Great PR, not tested yet.

editoast/src/generated_data/sql/generate_operational_point_layer.sql Outdated Show resolved Hide resolved
editoast/editoast_models/src/tables.rs Outdated Show resolved Hide resolved
editoast/migrations/2024-11-25-111024_add_track_field_operational_point_layer/up.sql Outdated Show resolved Hide resolved
editoast/editoast_schemas/src/train_schedule/path_item.rs Outdated Show resolved Hide resolved
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch from 78a0f76 to 8078784 Compare November 27, 2024 10:21
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch 3 times, most recently from 2a7585f to db6a27d Compare November 27, 2024 12:13
flomonster
flomonster requested changes Nov 27, 2024
View reviewed changes
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.

  • We should at least add one test with a track reference.
  • Nitpick: We are using track_name in the infra format. Why using track_label for the train schedule?

Here is a train_schedules.json I used to test it.

editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch 4 times, most recently from d70428f to ea32b24 Compare November 27, 2024 23:14
@younesschrifi
editoast: refacto path item location to add track and label information
e06f44c 
Signed-off-by: Youness CHRIFI ALAOUI <youness.chrifi@gmail.com>
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch from ea32b24 to f5fcbf9 Compare November 27, 2024 23:15
flomonster
flomonster approved these changes Nov 28, 2024
View reviewed changes
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

editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/path_item_cache.rs Outdated Show resolved Hide resolved
editoast/src/views/path/pathfinding.rs Outdated Show resolved Hide resolved
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch 2 times, most recently from 2168f09 to 43810c4 Compare November 28, 2024 13:05
@younesschrifi
editoast: add new track_section field in operational point layer
e126d29 
Signed-off-by: Youness CHRIFI ALAOUI <youness.chrifi@gmail.com>
@younesschrifi younesschrifi force-pushed the yci/refacto-path-item-location branch from 43810c4 to e126d29 Compare November 28, 2024 13:27
@younesschrifi younesschrifi added this pull request to the merge queue Nov 28, 2024
Merged via the queue into dev with commit ced78b4 Nov 28, 2024
27 checks passed
@younesschrifi younesschrifi deleted the yci/refacto-path-item-location branch November 28, 2024 14:22
@theocrsb theocrsb mentioned this pull request Nov 28, 2024
Merged
@younesschrifi younesschrifi mentioned this pull request Dec 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leovalais leovalais leovalais left review comments

@flomonster flomonster flomonster approved these changes

@woshilapin woshilapin woshilapin approved these changes

Assignees
No one assigned
Labels
area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@younesschrifi @codecov-commenter @woshilapin @flomonster @leovalais