Move consistency checkers to a class #181
Conversation
fonsecadeline
force-pushed
the
move_consistency_checkers
branch
from
6f0f04e
to
d50fe81
Compare
fonsecadeline
force-pushed
the
move_consistency_checkers
branch
from
d50fe81
to
21ebd2e
Compare
fonsecadeline
force-pushed
the
move_consistency_checkers
branch
5 times, most recently
from
7b865aa
to
2e00eea
Compare
fonsecadeline
force-pushed
the
move_consistency_checkers
branch
from
2e00eea
to
f06c8d5
Compare
fonsecadeline
force-pushed
the
move_consistency_checkers
branch
from
f06c8d5
to
4702368
Compare
| @@ -124,7 +126,7 @@ def self.create(hash, delete = true) | |||
| vrp = super({}) | |||
| self.filter(hash) # TODO : add filters.rb here | |||
| # moved filter here to make sure we do have schedule_indices (not date) to do work_day check with lapses | |||
| self.check_consistency(hash) | |||
There was a problem hiding this comment.
It doesn't really concern the VRP, but the hash. I am not convinced at all that it should be a concern.
There was a problem hiding this comment.
What would you suggest ? We had previously created a class that validates Hash.
There was a problem hiding this comment.
We have in input a JSON received by grape, which converts it into an hash, this hash is then converted into a vrp model using active_hash.
My assumption is that, the intermediate hash should have is own class which inherit from hash and add the concern to it.
so we could directly call hash.check_consistency
There was a problem hiding this comment.
@fonsecadeline the class you proposed was completely different -- it was in Models module and inhereted from Vrp class, -- the class @braktar suggests is a lot straightforward and it is kind of a wrapper around Hash class so that you can add the concern to that class.
I am still not convinced that it worth the effort because the main reason we have these validations like this (i.e., manually) is that the validation functionality validates_x_of_y of active_hash(?) was broken in the previous Ruby version. We still haven't tested if it is still broken or not in the current Ruby version. This functionality also lets us define custom validators and it calls them automatically, and (if I am not mistaken) it gives clear fail messages with indexing etc. so our manual validators are temporary at best.
This would make more sense if we could use this custom Hash to tell Grape to use as validator (kinda like presenter + validation) but I don't think we can do that.
There was a problem hiding this comment.
I agree that this is meant to be temporary and maybe we do not need to spend too much time trying to make it in the best way.
What should I do then ? are we okay to keep it like that ? (#192)
|
Split_clustering_tests fail because we added a check : there can not be more than one visit if there is no schedule. Visits are considered a little differently in max_split algo so 7 tests fail now. |
|
Real scheduling cases should be okay after we merge #191 |
VRP validation was getting too long and mixed. I think we had discussed doing this in the meantime of [I do not remember what].
In any case that would help to have a cleaner base for future developments