front: hourly timetable adapt nge - #17973
Conversation
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
Here are a few comments. Note, I haven't fully reviewed the PR yet.
| ) | ||
| return null; | ||
| const offset = timeLock.consecutiveTime - startTimeLock.consecutiveTime; | ||
| return new Date(startDate.getTime() + offset * 60 * 1000); |
There was a problem hiding this comment.
Nice, no more manual duration manipulation! :)
| // eslint-disable-next-line import/extensions | ||
| import duration from 'dayjs/plugin/duration.js'; | ||
|
|
||
| import type { TimetableType } from 'common/api/osrdEditoastApi'; |
There was a problem hiding this comment.
I'm not a fan of "tainting" duration helpers with something specific to operational studies. Can we find a better home for this function?
| * Elapsed time between two start times of the same kind. The result is a Duration in both | ||
| * cases: an elapsed time is an elapsed time, whatever the reference the operands count from. | ||
| */ | ||
| export const subtractStartTimes = (a: StartTime, b: StartTime) => |
There was a problem hiding this comment.
This allows subtracting a Date from a Duration, and the other way around. Can we check that both values have the same type?
| ): T => { | ||
| if (timetableType !== 'HOURLY' || !paced) return pathAndSchedule; | ||
| const intervalMs = Duration.parse(paced.interval).ms; | ||
| if (intervalMs <= 0) return pathAndSchedule; |
| * Taking the offset modulo the interval designates the very same occurrence: with a train | ||
| * every 30 min, departing at :59 and departing at :29 are the same train. | ||
| */ | ||
| export const clampOffsetToInterval = <T extends { start_time: number }>( |
There was a problem hiding this comment.
Can we fold this (keeping a comment explaining the reasoning) inside calculateStartTime()? It seems easy to forget to call this function. This would also remove the unnecessary generic.
| }); | ||
|
|
||
| describe('generatePathAndSchedule', () => { | ||
| test.each(FIXTURES)('hourly timetable: ngeToOsrd-inputDto-$0', async (name) => { |
There was a problem hiding this comment.
It seems weird to have a kind of generic test which does the same checks across random trainruns. Test should be "dumb", with hardcoded values.
Can't we instead pick a single trainrun, and check that the start_time is what we expect?
|
I've introduced a bunch of helpers in #18076 which overlap with this PR's. |
4 participants
close #17634