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 @@
## peb/editoast/add_etcs_brake_params_to_rs_model #10273 +/- ##
==================================================================================
+ Coverage 81.44% 81.70% +0.25%
==================================================================================
Files 1060 778 -282
Lines 104354 76128 -28226
Branches 722 722
==================================================================================
- Hits 84994 62198 -22796
+ Misses 19319 13889 -5430
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
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é <[email protected]>
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
After #10147 (comment)
TODO: