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: provide details when pathfinding failed #9162

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

younesschrifi
Copy link
Contributor

@younesschrifi younesschrifi commented Oct 2, 2024

Close #8038

@github-actions github-actions bot added the area:editoast Work on Editoast Service label Oct 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

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

Codecov Report

Attention: Patch coverage is 22.93578% with 84 lines in your changes missing coverage. Please review.

Project coverage is 39.06%. Comparing base (698b8f4) to head (914365c).
Report is 35 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/src/views/path/pathfinding.rs 22.58% 48 Missing ⚠️
...nt/src/modules/pathfinding/hooks/usePathfinding.ts 0.00% 15 Missing ⚠️
editoast/src/views/train_schedule.rs 0.00% 10 Missing ⚠️
...nalStudies/helpers/formatTrainScheduleSummaries.ts 0.00% 5 Missing ⚠️
editoast/src/views/timetable/stdcm.rs 0.00% 2 Missing ⚠️
...dules/trainschedule/components/Timetable/consts.ts 0.00% 1 Missing and 1 partial ⚠️
...odules/trainschedule/components/Timetable/types.ts 0.00% 1 Missing ⚠️
...odules/trainschedule/components/Timetable/utils.ts 0.00% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #9162      +/-   ##
============================================
- Coverage     39.13%   39.06%   -0.08%     
- Complexity     2267     2270       +3     
============================================
  Files          1305     1308       +3     
  Lines         99068    99315     +247     
  Branches       3310     3316       +6     
============================================
+ Hits          38771    38796      +25     
- Misses        58336    58554     +218     
- Partials       1961     1965       +4     
Flag Coverage Δ
core 74.99% <100.00%> (+0.02%) ⬆️
editoast 71.89% <28.57%> (-0.13%) ⬇️
front 10.32% <0.00%> (-0.04%) ⬇️
gateway 2.50% <ø> (ø)
osrdyne 3.29% <ø> (-0.01%) ⬇️
railjson_generator 87.49% <ø> (ø)
tests 86.71% <ø> (ø)

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/provide_details_to_pathfinding_not_found branch 8 times, most recently from c72891a to 124328d Compare October 10, 2024 21:53
@younesschrifi younesschrifi marked this pull request as ready for review October 11, 2024 12:53
@younesschrifi younesschrifi requested review from a team as code owners October 11, 2024 12:53
@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch from 124328d to 17485fe Compare October 11, 2024 12:57
@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch 5 times, most recently from 2e1289d to 5f6ad70 Compare October 15, 2024 07:42
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.

In general, I realize that every (or almost every) struct in the module pathfinding are prefixed with Pathfinding which is somewhat redundant. We might want to fix that but in another PR (hence I put a comment that don't need to be resolved). cc @leovalais

Apart from that, thank you for this PR, I'm always amazed by what this application will bring to debuggability of pathfinding (and simulation) errors.

@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch from 5f6ad70 to 484797d Compare October 15, 2024 12:30
@younesschrifi younesschrifi requested a review from a team as a code owner October 15, 2024 12:30
@younesschrifi younesschrifi requested a review from eckter October 15, 2024 12:30
Copy link
Contributor

@leovalais leovalais 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 this PR! LGTM on the views side, but I think there's too much logic in the core module. I agree with @woshilapin about the redundancy of the prefix, and I'm fine with changing that in another PR. I'll test this later.

Copy link
Contributor

@Akctarus Akctarus left a comment

Choose a reason for hiding this comment

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

Tested, left few comments

@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch 4 times, most recently from bc525a8 to 068286f Compare October 17, 2024 10:15
Copy link
Contributor

@Akctarus Akctarus 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 the front part, not tested yet

@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch 4 times, most recently from c57e33a to 58107e9 Compare October 21, 2024 15:17
@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch 2 times, most recently from 3002043 to eb54577 Compare October 21, 2024 15:24
Copy link
Contributor

@Akctarus Akctarus left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Tested and it works great ! Left some comment

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.

Core part was very complex, good job navigating through it, thanks for that.. :D

@younesschrifi younesschrifi requested review from SharglutDev and removed request for eckter October 22, 2024 14:49
@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch from b61e346 to bdf6115 Compare October 22, 2024 15:11
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 and tested, good job. You can resolve the last 2 comments when needed :)

Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes! A few last modifications 🙏:

@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch 2 times, most recently from ecb8a3b to e28f952 Compare October 23, 2024 19:57
Signed-off-by: Youness CHRIFI ALAOUI <[email protected]>
Signed-off-by: Youness CHRIFI ALAOUI <[email protected]>
@younesschrifi younesschrifi force-pushed the yci/provide_details_to_pathfinding_not_found branch from e28f952 to 914365c Compare October 23, 2024 20:05
@younesschrifi younesschrifi added this pull request to the merge queue Oct 23, 2024
Merged via the queue into dev with commit 74968f6 Oct 23, 2024
24 checks passed
@younesschrifi younesschrifi deleted the yci/provide_details_to_pathfinding_not_found branch October 23, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:editoast Work on Editoast Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

editoast: error pathfinding_not_found has not the details provided by core
8 participants