Conversation
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
Nice work on the sortPorts() cleanup!
Can the cleaned up files be added to tsconfig.strictNullChecks.json, to ensure they don't regress?
| connections: ConnectionDto[]; // all connections aligned to the node | ||
|
|
||
| resourceId: number; // reference to the algined (resource - not yet implemented) | ||
| resourceId: number | null; // reference to the algined (resource - not yet implemented) |
There was a problem hiding this comment.
Could we use | undefined here instead of | null? For DTO specifically, it better reflects that old JSON files are missing this field.
| } | ||
|
|
||
| getPort(portId: number): Port { | ||
| getPort(portId: number | undefined): Port | undefined { |
There was a problem hiding this comment.
It sounds a bit weird to accept undefined here - any reason why we need to?
|
|
||
| getNextTrainrunSection(trainrunSection: TrainrunSection): TrainrunSection { | ||
| getNextTrainrunSection( | ||
| trainrunSection: TrainrunSection | undefined | null, |
| }); | ||
| if (transition !== undefined) { | ||
| return this.getPort(transition.getPortId1()).getTrainrunSection(); | ||
| return this.getPort(transition.getPortId1())?.getTrainrunSection() ?? undefined; |
There was a problem hiding this comment.
Doesn't getPort() return undefined already?
| ): TrainrunSection | undefined { | ||
| const portsForTrainrun = this.ports.filter( | ||
| (port) => port.getTrainrunSection().getTrainrunId() === trainrunId, | ||
| (port) => port?.getTrainrunSection()?.getTrainrunId() === trainrunId, |
There was a problem hiding this comment.
port should never be undefined here.
| ): TrainrunSection | undefined { | ||
| const portsForTrainrun = this.ports.filter( | ||
| (port) => port.getTrainrunSection().getTrainrunId() === trainrunId, | ||
| (port) => port?.getTrainrunSection()?.getTrainrunId() === trainrunId, |
There was a problem hiding this comment.
It sounds weird to have a port not be attached to any trainrun section. Can this really happen?
If it's not supposed to happen, can we null-assert or throw an error in getTrainrunSection() instead?
| mergeConnections( | ||
| netzgrafikDto: NetzgrafikDto, | ||
| trainrunSectionMap: Map<number, number>, | ||
| trainrunSectionMap: Map<number | undefined, number>, |
There was a problem hiding this comment.
In which situation does it make sense to have undefined as the Map key?
(Wouldn't that result in key collisions anyways?)
| } | ||
|
|
||
| getNodeFromId(nodeId: number): Node { | ||
| getNodeFromId(nodeId: number | undefined): Node | undefined { |
There was a problem hiding this comment.
Here too, I don't think we should accept an undefined ID.
3 participants
Properly handle null and undefined values in many functions without lying on the types, and without ignoring the possibility of such values which can lead to crashes (such as OpenRailAssociation/osrd#14085 which this pr fixes)
There are a few arbitrary decisions taken in this pr which can be debated. In particular, how much we want to throw or use "!" VS how much we want to fallback or use "?". I generally tried to assume that getters could be called before init and so avoided as much as possible to fail on null values for these, but for a few setters I was more lax. The fallback values when sorting are a bit ugly too as a workaround.
The pr can be checked by setting
"strictNullChecks": truein tsconfig.json. All edited files should have 0 errors after this pr, except node.service which is only partially handled by this pr.