Clippy fixes #295
Clippy fixes #295
Conversation
|
Not sure why the commits for the rust fmt workflow are included in this, let me know if I can fix it up somehow. |
|
It looks like you merged rather than fast forwarded: https://code-maven.com/enforce-fast-forward-as-merge-strategy I suspect you ran into issues because I recently changed the You could also fetch the latest changes, and then reset --hard to |
| @@ -88,7 +88,7 @@ pub fn get_defaults(path: &str) -> Result<TestDefaults, io::Error> { | |||
| Ok(serde_yaml::from_str(&s).expect("Parsing YAML failed.")) | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Serialize, Deserialize, Clone)] | |||
| #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] | |||
There was a problem hiding this comment.
Question: Which clippy rule is this? It seems fine since these are in tests.
There was a problem hiding this comment.
https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq
For tests yeah, I'd say this rule is not applicable, we can suppress the warning
| @@ -2,7 +2,7 @@ use fluent_bundle::FluentError; | |||
| use std::error::Error; | |||
| use unic_langid::LanguageIdentifier; | |||
|
|
|||
| #[derive(Debug, PartialEq)] | |||
| #[derive(Debug, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Question (blocking): Similar to above, I'd like to know what this clippy rule is before merging, since it's changing a public facing API.
There was a problem hiding this comment.
https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq
This is more of a judgement call tbh, is there a reason to limit it to PartialEq and not Eq?
| @@ -53,7 +53,7 @@ pub enum ResourceType { | |||
| } | |||
|
|
|||
| /// A resource identifier for a localization resource. | |||
| #[derive(Debug, Clone, Hash)] | |||
There was a problem hiding this comment.
Question (blocking): Which clippy rule is this? I don't understand this change (probably since I don't know the clippy rule)
There was a problem hiding this comment.
https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
There's an impl PartialEq which only uses self.value for equality testing, while the derived Hash would use both value & resource_type, which breaks the invariant k1 == k2 => hash(k1) == hash(k2) (from the clippy rule)
| @@ -5,15 +5,15 @@ use super::Comment; | |||
| // This is a helper struct used to properly deserialize referential | |||
| // JSON comments which are single continous String, into a vec of | |||
| // content slices. | |||
| #[derive(Debug, PartialEq, Clone)] | |||
| #[derive(Debug, PartialEq, Eq, Clone)] | |||
There was a problem hiding this comment.
TODO (me): Review this and any other Eq traits after the above questions are answered.
There was a problem hiding this comment.
|
Looks like it's difficult to sync the fork after the branch name change - will resubmit a new PR. |
Fixing up clippy warnings/errors before we look to address #287