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

table, trains after midnight #8334

Merged
merged 4 commits into from
Sep 17, 2024
Merged

table, trains after midnight #8334

merged 4 commits into from
Sep 17, 2024

Conversation

anisometropie
Copy link
Contributor

@anisometropie anisometropie commented Aug 5, 2024

closes #7781

We now use a different format to represent arrival and departure in SuggestedOP and PathStep, an ISO duration string. The previous time string format hh:mm:ss was insufficient for distinguishing times spanning more than 24 hours

These two types are used in different places beyond the scope of just the input/outpt, we should test everything that uses these two types

@anisometropie anisometropie requested a review from a team as a code owner August 5, 2024 14:42
@anisometropie anisometropie marked this pull request as draft August 5, 2024 14:43
@anisometropie anisometropie changed the title s table, trains after midnight Aug 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2024

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

Codecov Report

Attention: Patch coverage is 48.15618% with 239 lines in your changes missing coverage. Please review.

Project coverage is 37.05%. Comparing base (2d6138c) to head (143da9a).
Report is 4 commits behind head on dev.

Files with missing lines Patch % Lines
front/src/modules/timesStops/helpers/utils.ts 75.67% 19 Missing and 26 partials ⚠️
front/src/modules/timesStops/TimesStops.tsx 0.00% 39 Missing ⚠️
front/src/modules/timesStops/TimeInput.tsx 0.00% 29 Missing ⚠️
front/src/modules/timesStops/ReadOnlyTime.tsx 0.00% 22 Missing and 1 partial ⚠️
...rc/modules/timesStops/hooks/useTimeStopsColumns.ts 0.00% 18 Missing ⚠️
front/src/common/types.ts 0.00% 15 Missing ⚠️
...ont/src/modules/timesStops/helpers/scheduleData.ts 28.57% 12 Missing and 3 partials ⚠️
front/src/modules/pathfinding/utils.ts 41.17% 5 Missing and 5 partials ⚠️
front/src/modules/timesStops/TimesStopsInput.tsx 0.00% 9 Missing ⚠️
...src/modules/timesStops/hooks/useOutputTableData.ts 0.00% 9 Missing ⚠️
... and 11 more

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #8334      +/-   ##
============================================
+ Coverage     36.98%   37.05%   +0.06%     
  Complexity     2211     2211              
============================================
  Files          1259     1260       +1     
  Lines        114565   114820     +255     
  Branches       3192     3223      +31     
============================================
+ Hits          42374    42546     +172     
- Misses        70290    70342      +52     
- Partials       1901     1932      +31     
Flag Coverage Δ
core 74.79% <ø> (ø)
editoast 72.44% <ø> (-0.01%) ⬇️
front 15.17% <48.15%> (+0.20%) ⬆️
gateway 2.20% <ø> (ø)
osrdyne 2.60% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.46% <ø> (ø)

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.

@anisometropie anisometropie force-pushed the vcs/trains-over-midnight branch 4 times, most recently from 7e2b8f5 to 7354ddf Compare August 9, 2024 08:16
@anisometropie anisometropie force-pushed the vcs/trains-over-midnight branch 6 times, most recently from 302df29 to 74edc28 Compare August 13, 2024 05:22
@anisometropie anisometropie marked this pull request as ready for review August 13, 2024 07:33
@theocrsb
Copy link
Contributor

image When I try to add 1 hour to 23 hours the requested departure time is not displayed. It is displayed when it is not midnight.

@theocrsb
Copy link
Contributor

image I don't know if this use case should be possible, but when I try to start the train 2 days later (172800 seconds), the requested departure time only shows J+1.

front/src/modules/timesStops/ReadOnlyTime.tsx Outdated Show resolved Hide resolved
front/src/modules/timesStops/TimeInput.tsx Outdated Show resolved Hide resolved
@theocrsb theocrsb requested a review from clarani August 14, 2024 09:56
@anisometropie anisometropie force-pushed the vcs/trains-over-midnight branch 3 times, most recently from 0fa1f84 to 4fef754 Compare September 3, 2024 10:01
@anisometropie
Copy link
Contributor Author

image I don't know if this use case should be possible, but when I try to start the train 2 days later (172800 seconds), the requested departure time only shows J+1.

It’s not a case we currently choose to handle

@anisometropie
Copy link
Contributor Author

anisometropie commented Sep 13, 2024

TBH I am not a huge fan of these new types which don't improve type safety (since they're aliases)…

@emersion

They are meant to define clearly what they are, and give move details when you hover your mouse over them.

I don’t want to lose the info and go back to a string. they can easily be changed with better types if we know where they are

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 work :)

@anisometropie anisometropie dismissed theocrsb’s stale review September 17, 2024 16:37

on vacation, all the changes done !

@anisometropie anisometropie force-pushed the vcs/trains-over-midnight branch 2 times, most recently from fba5654 to 5a2c9e0 Compare September 17, 2024 16:42
@anisometropie anisometropie added this pull request to the merge queue Sep 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2024
No change in behavior, just testing the previous behavior of the simulation input table, when a value changes.

Signed-off-by: Valentin Chanas <[email protected]>
- using lodash pick to simplify the object construction
- no need to lookup twice to find the corresponding pathStep
- we don’t need to create copies of the pathSteps array since we are using immer to handle immutability for us
- extracting the reducer logic in buildPathStepFromSuggestedOp to be reused

Signed-off-by: Valentin Chanas <[email protected]>
@anisometropie anisometropie added this pull request to the merge queue Sep 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2024
@anisometropie anisometropie added this pull request to the merge queue Sep 17, 2024
Merged via the queue into dev with commit 3460672 Sep 17, 2024
23 checks passed
@anisometropie anisometropie deleted the vcs/trains-over-midnight branch September 17, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSV2: input table: support trains that go over midnight
8 participants