Dijkstra_map_for_Godot icon indicating copy to clipboard operation
Dijkstra_map_for_Godot copied to clipboard

Remote Feature

Open astrale-sharp opened this issue 1 year ago • 5 comments

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

astrale-sharp avatar Mar 20 '23 10:03 astrale-sharp

Also note that you cannot undo a remove connection yet, it should be easy to implement.

astrale-sharp avatar Mar 20 '23 10:03 astrale-sharp

Also note that Documentation/Comments surrounding this PR could be improved

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.

astrale-sharp avatar Mar 20 '23 10:03 astrale-sharp

The more I look at it, the more I wonder if the macro enum_operation is truly necessary. Could we not just use DijkstraMap::apply_operation everywhere ?

Maybe then you'll say that it will be hard keeping Remote and DijkstraMap in sync, but consider this : if we only use apply_operation, we only need one method on Remote:

impl Remote {
    pub fn apply_operation(&mut self, operation: Operation) { // bikeshiddable name, of course
        self.operations.push(operation);
    }
}

arnaudgolfouse avatar Mar 22 '23 14:03 arnaudgolfouse

Btw, there are some clippy-related warnings still fired by the crate in this state.

arnaudgolfouse avatar Mar 22 '23 14:03 arnaudgolfouse

As we discussed, I put an alternative design here : https://github.com/arnaudgolfouse/Dijkstra_map_for_Godot/tree/alternative-operation-design Feel free to take whatever interests you !

arnaudgolfouse avatar Mar 30 '23 09:03 arnaudgolfouse