Introduce TryMapCoordsInplace algorithm.
#800
Conversation
| /// ); | ||
| /// # Ok::<(), &str>(()) | ||
| /// ``` | ||
| fn try_map_coords_inplace( |
There was a problem hiding this comment.
| fn try_map_coords_inplace( | |
| fn try_map_coords_in_place( |
I think "In place" should be spelled as two words, like: https://doc.rust-lang.org/std/ptr/fn.drop_in_place.html?search=in_place (7 matches)
There was a problem hiding this comment.
Oh... I see you were probably just matching the existing spelling of MapCoordsInplace. So now it's kind of murky "Internal Consistency vs. External Consistency" value proposition. 🤷
There was a problem hiding this comment.
Wild addendum: We have a pattern in this module of "one method per trait": TryMapCoordsInplace, MapCoords, TryMapCoords, MapCoordsInplace
I think it might be more convenient for users to have all these grouped under a single MapCoords trait. Is there any reason you can think of where it'd be good to keep them separate? What would you think about, instead of introducing this new TryMapCoordsInplace trait, instead deprecating TryMapCoords and MapCoordsInplace and consolidating all their methods (with corrected spelling) on the MapCoords trait?
Because you're following the existing pattern, I think it's reasonable to merge as is, so consider this non-blocking feedback.
If you'd prefer not to make these changes in this PR, I'll probably follow up with my own PR proposing something like what I outlined above.
There was a problem hiding this comment.
Regarding the naming, I noticed that too when I was writing this pull request. I named it this way to follow existing convention, but agree we should rename it. I opened this GitHub Issue to track that: #802
And regarding merging them into a single trait, I completely agree. There's already a separate issue for that: #721
| impl<T: CoordNum, E> TryMapCoordsInplace<T, E> for Triangle<T> { | ||
| fn try_map_coords_inplace( | ||
| &mut self, | ||
| func: impl Fn(&(T, T)) -> Result<(T, T), E>, |
There was a problem hiding this comment.
This is another artifact of history - but I'm curious - based on your usage, is it actually convenient to deal with (T, T) tuples here rather than Coordinate?
I think it makes sense to be consistent with all the other ones for now, but maybe something to consider for a future breaking change or alternate API.
There was a problem hiding this comment.
My preference is always to prefer working with structures with named fields when working with coordinates, as opposed to unnamed positional arguments. So yes, I do think it would be an improvement to change the tuple here to Coordinate. In fact we'll probably need to do that if/when the XYZM coordinate changes come in.
There was a problem hiding this comment.
I have a funny feeling that I'm responsible for this API, and that it's been somewhat inconvenient since the day it was introduced.
|
bors r=michaelkirk |
|
Build succeeded: |
CHANGES.mdif knowledge of this change could be valuable to users.Fixes #720.
This would be helpful in a couple places in rgis