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

front: refacto useOutputData #9642

Merged
merged 1 commit into from
Nov 27, 2024
Merged

front: refacto useOutputData #9642

merged 1 commit into from
Nov 27, 2024

Conversation

clarani
Copy link
Contributor

@clarani clarani commented Nov 7, 2024 •
edited
Loading

closes #8949
will also close #8848

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 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 28.13853% with 166 lines in your changes missing coverage. Please review.

Project coverage is 38.19%. Comparing base (e1bb669) to head (6593638).
Report is 9 commits behind head on dev.

Files with missing lines Patch % Lines
...src/modules/timesStops/hooks/useOutputTableData.ts 0.00% 84 Missing ⚠️
...ront/src/modules/timesStops/helpers/arrivalTime.ts 0.00% 50 Missing ⚠️
...ons/operationalStudies/views/SimulationResults.tsx 0.00% 10 Missing ⚠️
...ations/operationalStudies/hooks/useScenarioData.ts 0.00% 6 Missing ⚠️
...nalStudies/helpers/formatTrainScheduleSummaries.ts 0.00% 5 Missing ⚠️
...ont/src/modules/timesStops/helpers/scheduleData.ts 79.16% 5 Missing ⚠️
front/src/modules/timesStops/TimesStopsOutput.tsx 0.00% 3 Missing ⚠️
...t/src/modules/timesStops/helpers/computeMargins.ts 91.42% 3 Missing ⚠️

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

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #9642      +/-   ##
==========================================
- Coverage   38.22%   38.19%   -0.04%     
==========================================
  Files         995      995              
  Lines       91893    91917      +24     
  Branches     1189     1193       +4     
==========================================
- Hits        35127    35104      -23     
- Misses      56312    56358      +46     
- Partials      454      455       +1
Flag Coverage Δ
editoast 73.30% <ø> (-0.03%) ⬇️
front 20.19% <28.13%> (-0.04%) ⬇️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.74% <ø> (ø)

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.

@Synar Synar mentioned this pull request Nov 13, 2024
Merged
@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch 5 times, most recently from ac3a4af to e989762 Compare November 18, 2024 15:49
@clarani clarani requested review from Synar and SharglutDev November 18, 2024 15:51
@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch 2 times, most recently from 9dfbfc1 to 28d2249 Compare November 20, 2024 11:04
@clarani clarani marked this pull request as ready for review November 20, 2024 11:04
@clarani clarani requested a review from a team as a code owner November 20, 2024 11:04
Synar
Synar reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@Synar Synar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very nice PR! useOutputTableData was a mess, it's a lot more straightforward now

Edit : nevermind, I think the line I chose for testing is just bugged, but it seems to work fine for other lines, and possibly that issue has nothing to do with the pr. I'm checking ^^

Edit edit : yep the bug (waypoints in the wrong order with the wrong informations for paris gare de lyon when going from gare de lyon to lyon) is there on dev also. Sorry!

@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch from 28d2249 to 79322d9 Compare November 22, 2024 09:02
@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Nov 22, 2024
@Synar Synar mentioned this pull request Nov 22, 2024
Merged
SharglutDev
SharglutDev reviewed Nov 22, 2024
View reviewed changes
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.

Thank you for this great refacto, it's really easier to handle this hook now :)

Left some comments, feel free to contact me. to discuss some of them

SharglutDev
SharglutDev reviewed Nov 22, 2024
View reviewed changes
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.

I also have a weird thing here

Capture d’écran 2024-11-22 à 16 19 27

The row shouldn't be orange for 1sec difference or do I miss something ?

@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch from 56912ce to 30ff814 Compare November 26, 2024 12:07
SharglutDev
SharglutDev approved these changes Nov 26, 2024
View reviewed changes
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, great job !

@SharglutDev SharglutDev requested a review from Synar November 26, 2024 13:18
@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch from df49ba8 to 01dfea7 Compare November 26, 2024 13:29
Synar
Synar approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@Synar Synar 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 LGTM, great work!

@clarani
front: refacto useOutputData
6593638 
The logic to compute the data for the output table has been modified:

- instead of handling both pathsteps and regular operational points on the path at the same time, we now handle first the path steps (which may have some additional
information, such as margins, schedules, stop duration...) and then handle the regular operational points (we only need to compute the arrival time). This
simplifies a lot the hook

- instead of re-computing the time of each waypoint, we now use the times received in the summary of the train.

Signed-off-by: Clara Ni <[email protected]>
@clarani clarani force-pushed the cni/8949-refacto-useOutputTableData branch from 36ce995 to 6593638 Compare November 27, 2024 08:39
@Maymanaf Maymanaf self-requested a review November 27, 2024 10:19
Maymanaf
Maymanaf approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@Maymanaf Maymanaf 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 e2e tests

@clarani clarani added this pull request to the merge queue Nov 27, 2024
Merged via the queue into dev with commit ab6cdbb Nov 27, 2024
27 checks passed
@clarani clarani deleted the cni/8949-refacto-useOutputTableData branch November 27, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Synar Synar Synar approved these changes

@Maymanaf Maymanaf Maymanaf approved these changes

@SharglutDev SharglutDev SharglutDev approved these changes

Assignees
No one assigned
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Milestone
No milestone
5 participants
@clarani @codecov-commenter @Synar @Maymanaf @SharglutDev