-
Notifications
You must be signed in to change notification settings - Fork 46
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
editoast: add consist params validation #10332
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10332 +/- ##
==========================================
+ Coverage 81.83% 81.85% +0.01%
==========================================
Files 1081 1081
Lines 106879 106946 +67
Branches 721 721
==========================================
+ Hits 87466 87538 +72
+ Misses 19372 19367 -5
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2f8c41b
to
55eb68c
Compare
19d7604
to
a6f1f27
Compare
a6f1f27
to
60bf549
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing in the office, the main goal of the issue is to protect from unclear crash.
The front part already does the main job, so we should just protect the backend from unclear crashes, without narrowing use when it's technically OK (at a reasonable cost).
👍 Thanks for getting rid of a new part of the 'validator' use 🎉
60bf549
to
6681143
Compare
fa1d99d
to
e83746b
Compare
dcb9b9e
to
e7b1c75
Compare
866b407
to
e888917
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Feel free to resolve those 2 comments if applied.
e888917
to
9c09767
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm for front (not tested)
9c09767
to
71d0a09
Compare
b692e10
to
c1a380c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just please squash before merge 🙏
556da32
to
019cdc9
Compare
Signed-off-by: Egor Berezovskiy <[email protected]>
019cdc9
to
bfe451f
Compare
#[error("Invalid consist mass: {message}")] | ||
InvalidConsistMass { message: String }, | ||
#[error("Invalid consist length: {message}")] | ||
InvalidConsistLength { message: String }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have been great to have numerical values instead of a generic message string :/
Because of that, 1) the full error message is split in several places and 2) in tests you have to assert_eq
on strings instead of numerical values which are more stable.
fix #9543
Adding backend validation for the consists values to prevent calculation errors.