editoast: remove 'validator' dependency #10273
Conversation
bougue-pe
changed the title
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10273 +/- ##
==========================================
+ Coverage 81.58% 81.86% +0.28%
==========================================
Files 1062 763 -299
Lines 104970 76351 -28619
Branches 722 722
==========================================
- Hits 85636 62504 -23132
+ Misses 19293 13806 -5487
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. |
bougue-pe
force-pushed
the
peb/editoast/add_etcs_brake_params_to_rs_model
branch
from
fe27737 to
d1ab72f
Compare
bougue-pe
force-pushed
the
peb/editoast/rm_validator
branch
from
95c908a to
8fbe1f3
Compare
| effort_curves: &EffortCurves, | ||
| electrical_power_startup_time: Option<f64>, | ||
| raise_pantograph_time: Option<f64>, | ||
| ) -> std::result::Result<(), ValidationError> { | ||
| ) -> Result<(), D::Error> |
There was a problem hiding this comment.
I saw that it's part of your TODO list. The errors are going to change, but in the meantime, we might want to stick to the less ideal, but coherent way of doing: using crate::error::Result which uses InternalError.
There was a problem hiding this comment.
I think that this function being called by Deserialize::deserialize prevents from using InternalError
| let rolling_stock: RollingStock = | ||
| serde_json::from_reader(BufReader::new(rolling_stock_file))?; | ||
| let rolling_stock: Changeset<RollingStockModel> = rolling_stock.into(); |
There was a problem hiding this comment.
In the spirit of "parse don't validate", I'd argue that the validation should occur at the deserialization of the schema and not changeset (unlike what's currently being done on dev). There is an impl From<editoast_schema::RollingStock> for RollingStockChangeset existing in rolling_stock_model.rs. It might also be easier to pull the remote = "Self" trick on the schema as it's struct isn't generated.
Wdyt?
| effort_curves: &EffortCurves, | ||
| electrical_power_startup_time: Option<f64>, | ||
| raise_pantograph_time: Option<f64>, | ||
| ) -> std::result::Result<(), ValidationError> { | ||
| ) -> Result<(), D::Error> |
There was a problem hiding this comment.
I think that this function being called by Deserialize::deserialize prevents from using InternalError
bougue-pe
force-pushed
the
peb/editoast/add_etcs_brake_params_to_rs_model
branch
from
fb63ad4 to
85ccac8
Compare
TODO: * fix tests * use Changeset<RollingStock> and #[serde(remote = "Self")] if possible * maybe change error type returned by the free function * remove any 'validator' use Signed-off-by: Pierre-Etienne Bougué <bougue.pe@proton.me>
bougue-pe
force-pushed
the
peb/editoast/rm_validator
branch
from
8fbe1f3 to
090bcbc
Compare
After #10147 (comment)
TODO: