Remove manual map implementation in MaybeMath#161
Remove manual map implementation in MaybeMath#161alice-i-cecile merged 2 commits intoDioxusLabs:mainfrom
MaybeMath#161Conversation
TimJentzsch
added
code quality
TimJentzsch
changed the title
MaybeMath
|
I'm fine with this as long as we solve #158 at the same time :) |
Weibye
left a comment
Weibye
left a comment
There was a problem hiding this comment.
I'm fine with the changes as long as we add both some docs and tests that together explain exactly what they do. I've just started learning about .map these last days so my reading comprehension is not yet at a level where I look at the implementation and intuitively know what's up.
Hmm it's definitely a fair point that |
|
Looks like the tests are passing, now we still need to figure out if this is easier or harder to read/understand |
|
Given that this does the obvious thing + we have unit tests and docs now, I'm going to merge this. I don't love keeping clippy allows around either. |
3 participants
Objective
The implementations of
MaybeMathused#[allow(clippy::manual_map)].This PR removes this lint suppression and applies
cargo clippy --fixto usemapinstead of thematchstatement.Context
The original implementation justified the lint suppression as follows:
Feedback wanted
Do the explicit map statements really make it more clear what's going on?
Personally, I think the
mapstatements are also clear.If we're unsure if something works we should add unit tests instead.
I think it's good to suppress as little lints as possible.