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

front: fix macro trainruns frequencies #10448

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

louisgreiner
Copy link
Contributor

@louisgreiner louisgreiner commented Jan 20, 2025

After #9105, we've lost the feature the macro trainrun frequencies

This way of handling macro train frequencies will be revamp during https://github.com/osrd-project/osrd-confidential/issues/779

Closes #10389
Closes #10552

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Jan 20, 2025
@louisgreiner louisgreiner marked this pull request as ready for review January 20, 2025 14:39
@louisgreiner louisgreiner requested a review from a team as a code owner January 20, 2025 14:39
@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch from 978af53 to 6dff182 Compare January 20, 2025 14:46
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.88%. Comparing base (ce5d15b) to head (c9c33f8).
Report is 11 commits behind head on dev.

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

Additional details and impacted files
@@           Coverage Diff           @@
##              dev   #10448   +/-   ##
=======================================
  Coverage   81.87%   81.88%           
=======================================
  Files        1077     1078    +1     
  Lines      107088   107144   +56     
  Branches      724      724           
=======================================
+ Hits        87681    87736   +55     
- Misses      19367    19368    +1     
  Partials       40       40           
Flag Coverage Δ
editoast 74.18% <ø> (-0.01%) ⬇️
front 89.45% <100.00%> (+<0.01%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 88.14% <ø> (ø)

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.

@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch 2 times, most recently from 9eefc97 to e947e07 Compare January 22, 2025 09:31
@louisgreiner
Copy link
Contributor Author

Depends on #10486

@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch 3 times, most recently from 1857f69 to 2c1f9f5 Compare January 24, 2025 11:02
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.

Works great just some questions :)

@emersion
Copy link
Member

It seems like this feature got lost in #9105.

I think this PR still creates frequency:: labels in NGE (without using them in any trainrun)?

@louisgreiner
Copy link
Contributor Author

frequency:: labels should be shared in NGE if they are not either frequency::30, or frequency::60 or frequency::120.

The former feature allowed, for instance, frequency::10 to be shared in NGE, even tho we don't match this frequency with an actual NGE frequency

@emersion

@emersion
Copy link
Member

Yeah, what I meant was that I think frequency::30 and friends show up as a regular label in NGE with this PR.

@emersion
Copy link
Member

I'm not a huge fan of the RegExp-based label parsing. (BTW, it's also used in ngeToOsrd.)

Maybe this can all be consolidated in a single function, used everywhere instead of the RegExp and string checks?

const frequencyIdFromLabel = (label: string) => {
  if (!label.startsWith('frequency::')) return null;
  const n = parseInt(label.split('::', 2)[1], 10);
  const frequency = DEFAULT_TRAINRUN_FREQUENCIES.find((freq) => freq.frequency === n);
  return frequency?.id ?? null;
};

@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch from d08cdd0 to ae0c6c3 Compare January 28, 2025 13:13
@louisgreiner
Copy link
Contributor Author

Because I found this bug on the way, i'm also fixing it in a separate commit : #10552

@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch from fd7b6a4 to c8d9bd3 Compare January 28, 2025 14:13
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 :)

Copy link
Contributor

@Uriel-Sautron Uriel-Sautron left a comment

Choose a reason for hiding this comment

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

LGTM, tested.

Copy link
Contributor

@Uriel-Sautron Uriel-Sautron left a comment

Choose a reason for hiding this comment

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

LGTM, tested.

@louisgreiner louisgreiner force-pushed the lgr/fix-macro-trainruns-frequencies branch from c8d9bd3 to c9c33f8 Compare February 4, 2025 09:01
@louisgreiner louisgreiner added this pull request to the merge queue Feb 4, 2025
Merged via the queue into dev with commit b8c9ba4 Feb 4, 2025
27 checks passed
@louisgreiner louisgreiner deleted the lgr/fix-macro-trainruns-frequencies branch February 4, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nge: stop_for calculation is inverted nge: frequency feature is broken
5 participants