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

Clippy nursery cleanup #17

Closed
wants to merge 2 commits into from

Conversation

gkorland
Copy link
Contributor

No description provided.

@gkorland gkorland changed the title Gkorland clippy nursery Clippy nursery cleanup Jan 17, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #17 (3180793) into master (607027f) will decrease coverage by 0.30%.
The diff coverage is 39.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   47.92%   47.61%   -0.31%     
==========================================
  Files           9        9              
  Lines        2982     3001      +19     
==========================================
  Hits         1429     1429              
- Misses       1553     1572      +19     
Impacted Files Coverage Δ
src/de.rs 11.83% <0.00%> (-0.26%) ⬇️
src/value.rs 57.64% <13.33%> (+0.12%) ⬆️
src/array.rs 72.61% <83.33%> (ø)
src/number.rs 69.36% <83.33%> (ø)
src/object.rs 61.72% <100.00%> (ø)
src/string.rs 76.96% <100.00%> (ø)
src/macros.rs 25.00% <0.00%> (-8.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 607027f...3180793. Read the comment docs.

@@ -748,25 +748,22 @@ impl<'de> VariantAccess<'de> for VariantDeserializer<'de> {
type Error = Error;

fn unit_variant(self) -> Result<(), Error> {
Copy link
Owner

Choose a reason for hiding this comment

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

The changes in this file seem to harm readability IMO. The clippy lint that suggests these should maybe be adjusted...

Copy link

@dtolnay dtolnay Apr 30, 2022

Choose a reason for hiding this comment

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

I believe the relevant lint is option_if_let_else. I strongly dislike this lint too. I downgraded it from clippy's pedantic category (which is already for low-signal lints) to nursery in rust-lang/rust-clippy#7568, meaning it isn't suitable for use.

@gkorland's commit in this PR is titled "clean clippy nursery warnings" so it's likely they did something like -Wclippy::nursery. That is definitely not an intended use of nursery lints.

$ cargo clippy -- -Dclippy::option_if_let_else

error: use Option::map_or instead of an if let/else
   --> src/de.rs:751:9
    |
751 | /         if let Some(value) = self.value {
752 | |             Deserialize::deserialize(value)
753 | |         } else {
754 | |             Ok(())
755 | |         }
    | |_________^ help: try: `self.value.map_or(Ok(()), |value| Deserialize::deserialize(value))`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else

error: use Option::map_or_else instead of an if let/else
   --> src/de.rs:762:9
    |
762 | /         if let Some(value) = self.value {
763 | |             seed.deserialize(value)
764 | |         } else {
765 | |             Err(SError::invalid_type(
...   |
768 | |             ))
769 | |         }
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else
help: try
    |
762 ~         self.value.map_or_else(|| Err(SError::invalid_type(
763 +                 Unexpected::UnitVariant,
764 +                 &"newtype variant",
765 +             )), |value| seed.deserialize(value))
    |

@gkorland gkorland requested a review from Diggsey April 30, 2022 19:06
@Diggsey
Copy link
Owner

Diggsey commented Apr 30, 2022

Please don't re-request review without either replying to my previous review or pushing changes.

@Diggsey Diggsey closed this Apr 30, 2022
@gkorland gkorland deleted the gkorland-clippy-nursery branch May 2, 2022 06:45
@Diggsey
Copy link
Owner

Diggsey commented May 2, 2022

@gkorland

Please notice I did reply to your last comments but didn't get any reply from you.

There are no comments at all from you on this PR? Even this comment I quoted seems to have since been deleted by you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants