Remote Feature #120
Remote Feature #120
Conversation
|
Also note that you cannot undo a remove connection yet, it should be easy to implement. |
|
Also note that The method generated by the macro return a Result, the impl to return this result is correct but stupid, I doubt the use for Errors is really useful and this probably could be improved as well, open to suggestions. |
operation enum, undo feature new defaults with function signature like impl Into<Parameter> add DijkstraMap::get_connection
|
The more I look at it, the more I wonder if the macro Maybe then you'll say that it will be hard keeping impl Remote {
pub fn apply_operation(&mut self, operation: Operation) { // bikeshiddable name, of course
self.operations.push(operation);
}
} |
|
Btw, there are some clippy-related warnings still fired by the crate in this state. |
| } | ||
| } | ||
|
|
||
| impl Into<Directional> for Option<bool> { |
There was a problem hiding this comment.
I think those impl should not exist : they are fully redundant with .unwrap_or_default(), and I honestly had a hard time parsing some pieces of code.
For example, some code reads
self
.connect_points(*source, *target, Some(*weight), false))
.and(self.connect_points(*target, *source, Some(*weight), Some(false)))why does the first call use false, and the second Some(false) ?
| #[fn = set_terrain_for_point; err = PointNotFound;] | ||
| SetTerrainForPoint { | ||
| id: PointId, | ||
| ttype: TerrainType, |
There was a problem hiding this comment.
come on, we can't write terrain_type ? 😛
| match self { | ||
| Operation::AddPoint { id, terrain_type: _ } => Self::RemovePoint { id: *id }, | ||
| Operation::AddPointReplace { id, terrain_type : _ } => { | ||
| if map.has_point(*id) { |
There was a problem hiding this comment.
I assume that, when the point is already present, the semantics of AddPointReplace are "RemovePoint followed by AddPoint", right ?
But then, we can't actually reverse this, for the same reason we can't reverse RemovePoint!
| if matches!(*directional, Directional::Bidirectional) { | ||
| match self | ||
| .connect_points(*source, *target, Some(*weight), false) | ||
| .and(self.connect_points(*target, *source, Some(*weight), Some(false))) |
There was a problem hiding this comment.
connect_points calls apply_operation again and ends up in this same match arm, and I find these hoops a bit difficult to follow...
I propose doing this instead (effectively inlining those connect_points calls), how does that look to you ?
// target -> source
let (connections, reverse_connections) = self
.get_connections_and_reverse(*source, *target)
.map_err(|_| Errors::PointNotFound(PointNotFound))?;
connections.insert(*target, *weight);
reverse_connections.insert(*source, *weight);
if *directional == Directional::Bidirectional {
// source -> target
let (connections, reverse_connections) = self
.get_connections_and_reverse(*target, *source)
.map_err(|_| Errors::PointNotFound(PointNotFound))?;
connections.insert(*source, *weight);
reverse_connections.insert(*target, *weight);
}
Ok(None)There was a problem hiding this comment.
I see how the duplication is not ideal though... Maybe with a closure ?
| if matches!(*directional, Directional::Bidirectional) { | ||
| if self | ||
| .remove_connection(*source, *target, false) | ||
| .and(self.remove_connection(*target, *source, false)) |
There was a problem hiding this comment.
Same comment as ConnectPoints
| directional, | ||
| } => { | ||
| // let bidirectional = bidirectional.unwrap_or(true); | ||
| // let weight = weight.unwrap_or(Weight(1.0)); |
There was a problem hiding this comment.
Should these be removed ?
| // let weight = weight.unwrap_or_default(); | ||
| // let directional = directional.unwrap_or_default(); | ||
| // self.operations.push(Operation::ConnectPoints { | ||
| // source, |
There was a problem hiding this comment.
Remove these comments ?
| } | ||
| } | ||
|
|
||
| impl Into<Directional> for bool { |
There was a problem hiding this comment.
Oh I just noticed !
Did you know that bool::default() is false ?
This mean that with both of these impl there, we have
let directional_1: Directional = Directional::default();
let directional_2: Directional = bool::default().into();
assert!(directional_1 != directional_2);
This seems like a problem...
|
As we discussed, I put an alternative design here : https://github.com/arnaudgolfouse/Dijkstra_map_for_Godot/tree/alternative-operation-design |
Hi!!
I refactored the code around a macro and operation on the map.
This works and pass all the test and is more expressive but more complex (this was one of my first macro : a pain) although some of the new code could be improved, I'm also unsure about the naming.
If you're interested in giving me a review you're more than welcome !
@arnaudgolfouse @MatejSloboda