front: drop PathStep.ch #8633
Merged
front: drop PathStep.ch #8633
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emersion
force-pushed
the
emr/drop-path-step-ch
branch
3 times, most recently
from
8ca55d3 to
131a5b3
Compare
emersion
force-pushed
the
emr/drop-path-step-ch
branch
from
131a5b3 to
4a4ef04
Compare
emersion
force-pushed
the
emr/drop-path-step-ch
branch
from
4a4ef04 to
fae6a1c
Compare
This information is duplicated in PathItemLocation.secondary_code. This has several downsides. First, the field secondary_code is inherited in the PathStep type event if it's never set (we omit() it when receiving the train schedule path from the backend). As a result writing "pathStep.secondary_code" in code is valid even if it would never contain anything. Additionally, the field needs to be converted back and forth from/to ch when sending HTTP requests. Second, the field is always defined even for path steps for which a ch doesn't make sense. For instance, if the path step is an operational_point or is a TrackOffset, the ch property would still be defined (and always be empty). All of this makes it easy to trip over and use "ch" or "secondary_code" in contexts where it doesn't make sense. While this is easily discovered in testing when initially writing a feature, any subsequent refactoring may subtly break the rest of the code. To fix this, drop the "ch" field and only use the auto-generated "secondary_code" field. This forces any code making use of ch to verify (with the "in" keyword) that accessing the field makes sense. Signed-off-by: Simon Ser <[email protected]>
emersion
force-pushed
the
emr/drop-path-step-ch
branch
from
fae6a1c to
1f6c1f4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This information is duplicated in PathItemLocation.secondary_code. This has several downsides.
First, the field secondary_code is inherited in the PathStep type event if it's never set (we omit() it when receiving the train schedule path from the backend). As a result writing "pathStep.secondary_code" in code is valid even if it would never contain anything. Additionally, the field needs to be converted back and forth from/to ch when sending HTTP requests.
Second, the field is always defined even for path steps for which a ch doesn't make sense. For instance, if the path step is an operational_point or is a TrackOffset, the ch property would still be defined (and always be empty).
All of this makes it easy to trip over and use "ch" or "secondary_code" in contexts where it doesn't make sense. While this is easily discovered in testing when initially writing a feature, any subsequent refactoring may subtly break the rest of the code.
To fix this, drop the "ch" field and only use the auto-generated "secondary_code" field. This forces any code making use of ch to verify (with the "in" keyword) that accessing the field makes sense.
Depends on: