apply most clippy suggestions #6

Follpvosten · 2021-02-03T11:36:15Z

Hi again!
Here I just went through and applied all of the clippy suggestions that made sense in my opinion. Of course I'm open to still changing stuff if it doesn't fit the crate's style/standards.
There are two clippy warnings and one "error" still left:

warning: module has the same name as its containing module
 --> src/value/mod.rs:3:1
  |
3 | mod value;
  | ^^^^^^^^^^
  |
  = note: `#[warn(clippy::module_inception)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_inception

This one I think is really just a nitpick; clippy would probably like you to remove the nested value::value module and move the code to the top-level module, but I don't think there's really a point in that.

warning: this import is redundant
   --> src/util.rs:336:1
    |
336 | pub(crate) use cloneable_fn_trait;
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove it entirely
    |
    = note: `#[warn(clippy::single_component_path_imports)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports

This is a definite false positive and should probably be reported to clippy if it isn't already; this warning should never apply to macros.

error: you are deriving `Hash` but have implemented `PartialEq` explicitly
   --> src/profile.rs:9:32
    |
9   | #[derive(Debug, PartialEq, Eq, Hash, Clone, PartialOrd, Ord)]
    |                                ^^^^
    |
    = note: `#[deny(clippy::derive_hash_xor_eq)]` on by default
note: `PartialEq` implemented here
   --> src/profile.rs:267:1
    |
267 | / impl PartialEq<Profile> for &Profile {
268 | |     fn eq(&self, other: &Profile) -> bool {
269 | |         self.as_str() == other.as_str()
270 | |     }
271 | | }
    | |_^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

Not sure what to do about this one, but I doubt that it has any real impact.

Follpvosten · 2021-02-03T11:38:44Z

src/profile.rs

@@ -48,9 +48,9 @@ impl From<ProfileTag> for Option<Profile> {

 impl From<&Profile> for ProfileTag {
    fn from(profile: &Profile) -> Self {
-        if profile == &Profile::Default {


I personally think this would look better as a match, but that's not my decision I'd say

We can't do a traditionalmatch, unfortunately, as Profile is not an enum. However, we could do something like:

match profile { p if p == Profile::Default => ProfileTag::Default, ... }

I think that'd be great.

Follpvosten · 2021-02-03T11:40:02Z

src/util.rs

@@ -98,7 +98,7 @@ pub fn diff_paths<P, B>(path: P, base: B) -> Option<PathBuf>
        }
    }

-    Some(comps.iter().map(|c| c.as_os_str()).collect::<PathBuf>().into())
+    Some(comps.iter().map(|c| c.as_os_str()).collect())


It's up for debate if removing the turbofish here is a good idea. It might make it less clear, you'll have to look at the function signature to know the type it's collecting into.

I'm fine with it.

Follpvosten · 2021-02-03T11:40:57Z

src/value/parse.rs

-        ',' | '{' | '}' | '[' | ']' => false,
-        _ => true
-    }
+    !matches!(byte, ',' | '{' | '}' | '[' | ']')


I think this looks ugly, especially the !matches!. I'm open to changing it back or to something else.

I'm okay with it.

Follpvosten · 2021-02-03T11:42:08Z

src/value/value.rs

-        Value::String(Tag::Default, value.to_string().into())
+impl From<&str> for Value {
+    fn from(value: &str) -> Value {
+        Value::String(Tag::Default, value.to_string())


This was not actually a clippy suggestion, I saw myself that the lifetime could be elided here.

Follpvosten · 2021-02-03T11:43:52Z

src/figment.rs

@@ -287,7 +287,8 @@ impl Figment {
    /// ```
    pub fn extract_inner<'a, T: Deserialize<'a>>(&self, key: &str) -> Result<T> {
        let merged = self.merged().map_err(|e| e.resolved(self))?;
-        let inner = merged.find(key).ok_or(Kind::MissingField(key.to_string().into()))?;
+        let inner = merged.find(key)
+            .ok_or_else(|| Kind::MissingField(key.to_string().into()))?;


Here I'm not sure if the formatting was a good decision. Please tell me if I should change it.

Only line would be preferred.

Follpvosten · 2021-02-03T11:45:33Z

src/providers/data.rs

-            .map_err(|e| de::Error::custom(e))?;
-
-        Self::from_str(&source)
+        Self::from_str(&std::fs::read_to_string(path).map_err(de::Error::custom)?)


While this is a lot simpler, we could keep the source variable part to make it more readable, like let source = std::fs::read_to_string(path).map_err(de::Error::custom)?; Self::from_str(&source)

Let's do that.

SergioBenitez

These are great! Just some minor comments.

SergioBenitez · 2021-02-13T02:14:42Z

src/figment.rs

@@ -287,7 +287,8 @@ impl Figment {
    /// ```
    pub fn extract_inner<'a, T: Deserialize<'a>>(&self, key: &str) -> Result<T> {
        let merged = self.merged().map_err(|e| e.resolved(self))?;
-        let inner = merged.find(key).ok_or(Kind::MissingField(key.to_string().into()))?;
+        let inner = merged.find(key)
+            .ok_or_else(|| Kind::MissingField(key.to_string().into()))?;


Only line would be preferred.

SergioBenitez · 2021-02-13T02:15:44Z

src/profile.rs

@@ -48,9 +48,9 @@ impl From<ProfileTag> for Option<Profile> {

 impl From<&Profile> for ProfileTag {
    fn from(profile: &Profile) -> Self {
-        if profile == &Profile::Default {


We can't do a traditionalmatch, unfortunately, as Profile is not an enum. However, we could do something like:

match profile { p if p == Profile::Default => ProfileTag::Default, ... }

I think that'd be great.

SergioBenitez · 2021-02-13T02:15:59Z

src/providers/data.rs

-            .map_err(|e| de::Error::custom(e))?;
-
-        Self::from_str(&source)
+        Self::from_str(&std::fs::read_to_string(path).map_err(de::Error::custom)?)


Let's do that.

SergioBenitez · 2021-02-13T02:16:21Z

src/util.rs

@@ -98,7 +98,7 @@ pub fn diff_paths<P, B>(path: P, base: B) -> Option<PathBuf>
        }
    }

-    Some(comps.iter().map(|c| c.as_os_str()).collect::<PathBuf>().into())
+    Some(comps.iter().map(|c| c.as_os_str()).collect())


I'm fine with it.

SergioBenitez · 2021-02-13T02:16:38Z

src/value/parse.rs

-        ',' | '{' | '}' | '[' | ']' => false,
-        _ => true
-    }
+    !matches!(byte, ',' | '{' | '}' | '[' | ']')


I'm okay with it.

SergioBenitez · 2021-02-13T02:18:47Z

src/providers/data.rs

@@ -294,19 +293,13 @@ pub trait Format: Sized {

    /// Parses `string` as the data format `Self` as a `T` or returns an error
    /// if the `string` is an invalid `T`.
-    fn from_str<'de, T: DeserializeOwned>(string: &'de str) -> Result<T, Self::Error>;
+    fn from_str<T: DeserializeOwned>(string: &str) -> Result<T, Self::Error>;


I don't think this is backwards compatible.

Let's remove this change and just leave a note in the source code about changing it in the future.

Ah, because this is a trait that might be implemented by other crates. Got it.

Follpvosten · 2021-02-13T09:10:26Z

I've applied changes according to your request. I hope this looks good!

SergioBenitez · 2021-02-14T06:47:24Z

Thank you!

Follpvosten commented Feb 3, 2021

View reviewed changes

SergioBenitez requested changes Feb 13, 2021

View reviewed changes

Follpvosten force-pushed the apply-clippy-suggestions branch from 3ceab26 to 64a56fe Compare February 13, 2021 09:09

Follpvosten and others added 2 commits February 13, 2021 22:41

Apply clippy suggestions.

7ed26c6

Simply 'Figment::provide()' logic.

19ce965

SergioBenitez force-pushed the apply-clippy-suggestions branch from 64a56fe to 19ce965 Compare February 14, 2021 06:42

SergioBenitez merged commit 57f0139 into SergioBenitez:master Feb 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply most clippy suggestions #6

apply most clippy suggestions #6

Follpvosten commented Feb 3, 2021

Follpvosten Feb 3, 2021

SergioBenitez Feb 13, 2021

Follpvosten Feb 3, 2021

SergioBenitez Feb 13, 2021

Follpvosten Feb 3, 2021

SergioBenitez Feb 13, 2021

Follpvosten Feb 3, 2021

Follpvosten Feb 3, 2021

SergioBenitez Feb 13, 2021

Follpvosten Feb 3, 2021

SergioBenitez Feb 13, 2021

SergioBenitez left a comment

SergioBenitez Feb 13, 2021

SergioBenitez Feb 13, 2021

SergioBenitez Feb 13, 2021

SergioBenitez Feb 13, 2021

SergioBenitez Feb 13, 2021

SergioBenitez Feb 13, 2021

Follpvosten Feb 13, 2021

Follpvosten commented Feb 13, 2021

SergioBenitez commented Feb 14, 2021

apply most clippy suggestions #6

apply most clippy suggestions #6

Conversation

Follpvosten commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Follpvosten commented Feb 13, 2021

SergioBenitez commented Feb 14, 2021