MatejSloboda / Dijkstra_map_for_Godot

MIT License
77 stars 13 forks source link

Remote Feature #120

Open astrale-sharp opened 1 year ago

astrale-sharp commented 1 year ago

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 commented 1 year ago

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

astrale-sharp commented 1 year ago

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.

arnaudgolfouse commented 1 year ago

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 commented 1 year ago

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

arnaudgolfouse commented 1 year ago

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 !