search

pchampin / mownstr

MownStr: Maybe Owned String
1 stars 2 forks source link

Handle clippy lints in breaking change #10

Open mkatychev opened 1 week ago

mkatychev commented 1 week ago
pchampin commented 1 week ago

done in b1f4078 (part of #8)

pchampin commented 1 week ago

which links to

I would like to follow up with the clippy lints for the naming schema in a breaking: MownStr::from_str and MownStr::borrowed are too similar to stdlib methods and made debugging a crate that borrows from strings quite confusing.

I see how MownStr::from_str clashes with FromStr::from_str and how this can cause confusing compiler errors. Ok to change it. Suggestion for a better name?

For MownStr::borrowed, I don't see where the issue is comming from. The std lib does not seem to have a method with that name.

pchampin commented 1 week ago
  • Add #[expect(clippy::mutable_key_type)] for generic Keys that need to be Hash such as in HashMap<MownStr, V>

I don't really get what the issue is here. Could you please open a separate issue for this, with an example of code that causes the issue with clippy?

mkatychev commented 1 week ago
  • Add #[expect(clippy::mutable_key_type)] for generic Keys that need to be Hash such as in HashMap<MownStr, V>

I don't really get what the issue is here. Could you please open a separate issue for this, with an example of code that causes the issue with clippy?

The issue is the same as https://github.com/rust-lang/rust-clippy/issues/5812 When I tried holding a BTreeMap<MownStr, T>. I managed to address the issue on my side by having a .clippy.toml file in my workspace:

# https://github.com/ruma/ruma/blob/main/.clippy.toml
ignore-interior-mutability = ["mownstr::MownStr"]

I imagine others that use MownStr or sophia_iri::IriRef<MownStr> as a key in a map might run into the same issue so it would not hurt to document it.

The only reason bytes::Bytes does not trip this flag is because it is part of the default ignore-interior-mutability config in clippy for example.

mkatychev commented 1 week ago

I see how MownStr::from_str clashes with FromStr::from_str and how this can cause confusing compiler errors. Ok to change it. Suggestion for a better name?

Perhaps ::from_slice or ::from_ref?

pchampin commented 1 week ago

When I tried holding a BTreeMap<MownStr, T>.

I tried to put this in a test (with a HashMap rather than a BTreeMap, but I imagine the problem should be the same), and I didn't get any clippy warning... Are you using specific clippy options to encounter the problem? Can you maybe make a PR that exhibits the issue?

For such cases (checking that something compiles and does not raise linter warnings), I create a function in the test module, not annotated as test (no need to run it), but annotated as allow(dead_code).

pchampin commented 1 week ago

I see how MownStr::from_str clashes with FromStr::from_str and how this can cause confusing compiler errors. Ok to change it. Suggestion for a better name?

Perhaps ::from_slice or ::from_ref?

done in f9b8667

mkatychev commented 1 week ago

When I tried holding a BTreeMap<MownStr, T>.

I tried to put this in a test (with a HashMap rather than a BTreeMap, but I imagine the problem should be the same), and I didn't get any clippy warning... Are you using specific clippy options to encounter the problem? Can you maybe make a PR that exhibits the issue?

For such cases (checking that something compiles and does not raise linter warnings), I create a function in the test module, not annotated as test (no need to run it), but annotated as allow(dead_code).

This was my fault, it seems that this issue only came up in https://github.com/pchampin/mownstr/pull/7

mkatychev commented 1 week ago

Once https://github.com/pchampin/mownstr/pull/11 is reviewed, this can be closed.