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: enable power restrictions merge #10561

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

clarani
Copy link
Contributor

@clarani clarani commented Jan 29, 2025

closes #8850

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Jan 29, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2025

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

Codecov Report

Attention: Patch coverage is 74.24242% with 34 lines in your changes missing coverage. Please review.

Project coverage is 81.89%. Comparing base (716e6c8) to head (5e2dc72).

Files with missing lines Patch % Lines
.../operationalStudiesConf/powerRestrictionReducer.ts 2.85% 34 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10561      +/-   ##
==========================================
- Coverage   81.91%   81.89%   -0.02%     
==========================================
  Files        1079     1079              
  Lines      107233   107331      +98     
  Branches      729      729              
==========================================
+ Hits        87840    87901      +61     
- Misses      19353    19390      +37     
  Partials       40       40              
Flag Coverage Δ
editoast 74.20% <ø> (-0.01%) ⬇️
front 89.43% <74.24%> (-0.04%) ⬇️
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.

@clarani clarani force-pushed the cni/8850-merge-power-restrictions branch from cecdc25 to a487ade Compare February 5, 2025 15:11
@clarani clarani marked this pull request as ready for review February 5, 2025 15:12
@clarani clarani requested a review from a team as a code owner February 5, 2025 15:12
@clarani clarani requested review from Math-R and RomainValls February 5, 2025 15:12
@clarani clarani enabled auto-merge February 6, 2025 09:05
… restrction range

When editing the first power restriction range, an extra empty range was added (range from 0 to 0). This was a bug
and should not occur anymore.

Signed-off-by: Clara Ni <[email protected]>
@clarani clarani force-pushed the cni/8850-merge-power-restrictions branch from a487ade to 5e2dc72 Compare February 6, 2025 09:06
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.

(was just passing by, didn't review all the code)

);

let newPowerRestrictionRanges: PowerRestriction[] = [];
let newPathSteps: PathStep[] = [...state.pathSteps].map((step) => step!);
Copy link
Contributor

@SharglutDev SharglutDev Feb 6, 2025

Choose a reason for hiding this comment

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

nit : we can probably add an early check and throw if a path step is null because we don't have access to the power restriction tool if origin or destination are null (meaning there is no pathfinding)

Copy link
Contributor

@RomainValls RomainValls left a comment

Choose a reason for hiding this comment

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

interval.editor.mov

It seems if you cut the ranges multiple times and merge two of them, you're not able to merge the ranges with a position lower than the first cut.
It does work for the range positioned after, and also for ranges two steps below the first cut.

EDIT: tried again, and was able to merge ranges before the first cut (at least 2 steps before the first cut), except for the range that is right before the first cut.

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.

front: merge several power restriction codes is not possible anymore
4 participants