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

core, editoast: rename rolling stock to physics consist #9665

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

woshilapin
Copy link
Contributor

Now that consists are supported instead of a single traction engine (named rolling stock in the entire application), it's reasonable to rename all along up to core.

It's a follow up of #9106.

❓ In core, I changed PhysicsRollingStockModel into PhysicsConsistModel. But there is also RollingStockParser which converts a PhysicsConsistModel into a RollingStock: I'm not sure how far I should push the renaming. Is RollingStock only about physics? Because RollingStock contains also information about identifier, or supportedSignalingSystems which not really be about the physics of the rolling stock?

@woshilapin woshilapin requested review from a team as code owners November 8, 2024 21:56
@woshilapin woshilapin requested a review from Wadjetz November 8, 2024 21:56
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2024

⚠️ 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 38.20%. Comparing base (9dedb37) to head (35146ac).
Report is 263 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    #9665      +/-   ##
==========================================
- Coverage   38.21%   38.20%   -0.02%     
==========================================
  Files         992      992              
  Lines       92158    92159       +1     
  Branches     1186     1186              
==========================================
- Hits        35217    35207      -10     
- Misses      56490    56501      +11     
  Partials      451      451              
Flag Coverage Δ
editoast 73.34% <100.00%> (-0.04%) ⬇️
front 20.18% <ø> (ø)
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 87.00% <ø> (ø)

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.

@woshilapin woshilapin added area:core Work on Core Service component:rollingstock work related to rollingstock definitions severity:minor Minor severity bug area:editoast Work on Editoast Service module:stdcm Short-Term DCM kind:refacto-task Task related to Refactorization Epic component:simulation rust Pull requests that update Rust code labels Nov 8, 2024
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

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

LGTM

❓ In core, I changed PhysicsRollingStockModel into PhysicsConsistModel. But there is also RollingStockParser which converts a PhysicsConsistModel into a RollingStock: I'm not sure how far I should push the renaming. Is RollingStock only about physics? Because RollingStock contains also information about identifier, or supportedSignalingSystems which not really be about the physics of the rolling stock?

I feel like a "rolling stock" is supposed to be "anything that can move on tracks". There's some semantics issues around identifiers and such, but I feel like we'd need to think more carefully about what we precisely intent there, we'd need to take some time to discuss all of this.

We should probably do it eventually, but maybe not quite yet

@woshilapin
Copy link
Contributor Author

@eckter I agree that, literally, rolling stock means what you said. But this term comes with the history of OSRD which actually associate rolling stock with: "something that has traction capability". Therefore, rolling stock today can be misunderstood, and I'd prefer to move to these terms instead:

  1. traction engine (some part of editoast already adopted that),
  2. towed rolling stock (since this feature is recent, I believe it's already relatively homogeneous everywhere),
  3. consist

What do you think? And yes, we can discuss it.

Now that consists are supported instead of a single traction engine
(named rolling stock in the entire application), it's reasonable
to rename all along up to `core`.

Signed-off-by: Jean SIMARD <[email protected]>
@woshilapin woshilapin force-pushed the wsl/core/physics-consist branch from b13c2a4 to 35146ac Compare December 3, 2024 15:00
@eckter
Copy link
Contributor

eckter commented Jan 7, 2025

@woshilapin

Sorry I didn't really see the answer. I didn't mean to block this, it's fine as it is.

In the context of core, we don't really consider the distinction between "anything that moves on track" or "anything with traction", or even consists and such. We don't handle the database and we don't assemble anything together. So it doesn't really matter either way in this context, we can go with whatever is consistent with the rest of the codebase.

The distinction between RollingStock and PhysicsRollingStock is mostly "legacy", I think it's related to circular dependencies between modules. The string identifier isn't really used as far as I know.

@woshilapin woshilapin added this pull request to the merge queue Jan 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 7, 2025
@woshilapin woshilapin added this pull request to the merge queue Jan 7, 2025
Merged via the queue into dev with commit a4190de Jan 7, 2025
28 checks passed
@woshilapin woshilapin deleted the wsl/core/physics-consist branch January 7, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core Work on Core Service area:editoast Work on Editoast Service component:rollingstock work related to rollingstock definitions component:simulation kind:refacto-task Task related to Refactorization Epic module:stdcm Short-Term DCM rust Pull requests that update Rust code severity:minor Minor severity bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants