front, rmi: stdcm: send consist changes to rmi#16640
Conversation
github-actions
Bot
added
area:front
Castavo
left a comment
Castavo
left a comment
There was a problem hiding this comment.
LGTM to me overall, I'll have to test
| @@ -114,6 +114,31 @@ components: | |||
| description: Error message describing what went wrong | |||
| required: | |||
| - detail | |||
| ConsistChange: | |||
There was a problem hiding this comment.
Maybe this object could be a Consist, and could be included in the SimulationReport with an allOf ? It would reduce duplication (but at the cost of making SimulationReport harder to read)
What do you think ?
There was a problem hiding this comment.
I didn't know we could do that, thanks for the tip! I think I would slightly favor readability over code uniqueness in this case, but if you have a marked preference for the allOf version I'm fine with it too ^^
There was a problem hiding this comment.
I don't have a marked preference indeed
If you're not convinced either, let's keep the code simpler 👍
There was a problem hiding this comment.
Talked about it with @Khoyo, he convinced me that the allOf version is probably superior.
But also, shouldn't we simply drop the main consist from the simulation report, and add it in the first requested step? That would simplify both the model and the osrd-data code. Though this can be discussed and done later.
DucNg
left a comment
DucNg
left a comment
There was a problem hiding this comment.
Good for front and tested (I can see the consist_change inside the request payload)
Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
Synar
force-pushed
the
ali/send-consist-changes-to-raimi
branch
from
8d88307 to
09cd9f0
Compare
Castavo
left a comment
Castavo
left a comment
There was a problem hiding this comment.
LGTM
Tested with our implem of RaiMI, awesome
3 participants
Close #16505