Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove manual map implementation in MaybeMath #161

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

TimJentzsch
Copy link
Collaborator

Objective

The implementations of MaybeMath used #[allow(clippy::manual_map)].
This PR removes this lint suppression and applies cargo clippy --fix to use map instead of the match statement.

Context

The original implementation justified the lint suppression as follows:

The logic here is subtle and critical;
explicit match statements make it more clear what's going on

Feedback wanted

Do the explicit map statements really make it more clear what's going on?
Personally, I think the map statements 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.

@TimJentzsch TimJentzsch added code quality Make the code cleaner or prettier. controversial This work requires a heightened standard of review due to implementation or design complexity labels Jun 12, 2022
@TimJentzsch TimJentzsch changed the title Remove manual map implementation in maybe math Remove manual map implementation in MaybeMath Jun 12, 2022
@alice-i-cecile
Copy link
Collaborator

I'm fine with this as long as we solve #158 at the same time :)

Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@TimJentzsch
Copy link
Collaborator Author

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 .map is harder to understand for users new to Rust / functional programming, however I'm not sure if adding documentation from std functions to our code base is a good idea

@TimJentzsch
Copy link
Collaborator Author

Looks like the tests are passing, now we still need to figure out if this is easier or harder to read/understand

@alice-i-cecile
Copy link
Collaborator

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.

@alice-i-cecile alice-i-cecile merged commit e75a55f into DioxusLabs:main Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Make the code cleaner or prettier. controversial This work requires a heightened standard of review due to implementation or design complexity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants