import: handle importing old files with numberOfStops #596
Conversation
anisometropie
force-pushed
the
vcs/import-old-files
branch
from
f69b61d to
a8500c5
Compare
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
Thanks for the patch! Overall looks good, just a few comments below.
| const interpolatedPosition = new Vec2D( | ||
| node1.getPositionX() + (node2.getPositionX() - node1.getPositionX()) * 0.5, | ||
| node1.getPositionY() + (node2.getPositionY() - node1.getPositionY()) * 0.5, | ||
| ); | ||
| nodeIntermediate.setPosition(interpolatedPosition.getX(), interpolatedPosition.getY()); |
There was a problem hiding this comment.
I would find it slightly nicer to perform the action of setting the new intermediate node position in the caller. I find it surprising that this function moves any node.
There was a problem hiding this comment.
not really sure would it make sense to create a node in this function and delegate the responsibility of setting its position to whomever is calling.
this function creates one node from two that already have a position. it would make sense to create a node that also have a position.
There was a problem hiding this comment.
Oh, I thought this function wasn't creating any new node? Isn't the node ID passed as the last argument of this function? Isn't addEmptyNode() invoked by the caller?
If this function creates a new node, it would indeed make sense to set its position here as well. If this function doesn't create a new node, it would make more sense to not mutate any node from here.
Does that make sense?
clarani
left a comment
clarani
left a comment
There was a problem hiding this comment.
Nice work, LGTM overall, but I'm not a specialist of this codebase :)
| for (let i = 0; i < trainrunSection.numberOfStops; i++) { | ||
| const newNode = this.nodeService.addEmptyNode(); | ||
| const {trainrunSection2} = this.trainrunSectionService.replaceIntermediateStopWithNode( | ||
| currentSection.id, | ||
| 0, | ||
| newNode.getId(), | ||
| ); | ||
| currentSection = trainrunSection2.getDto(); | ||
| } |
There was a problem hiding this comment.
Would you mind create a function in the trainrunSectionService to add an intermediate stop on it ? I think it might be useful for the future PRs. Thanks 🙏
There was a problem hiding this comment.
that,s already what replaceIntermediateStopWithNode does.
does you mean moving the entire replaceLegacyNumberOfStopsWithRealNodes into trainrunSectionService ?
There was a problem hiding this comment.
Hum I think it's fine like this in the end 👍
Description
in the migration #538,
numberOfStopsis removed from the model. The PR allows to to import old files still containings it.During
loadNetzgrafikDto, we now replace numberOfStops with actual nodes in the graph.closes #592
closes OpenRailAssociation/osrd#13286
some unit tests likely to fail until the whole migration is done
Issues
#592
Checklist
documentation/