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

Handle clippy lints in breaking change #10

Closed
2 tasks done
mkatychev opened this issue Sep 10, 2024 · 9 comments
Closed
2 tasks done

Handle clippy lints in breaking change #10

mkatychev opened this issue Sep 10, 2024 · 9 comments

Comments

@mkatychev
Copy link
Contributor

mkatychev commented Sep 10, 2024

@pchampin
Copy link
Owner

done in b1f4078 (part of #8)

@pchampin
Copy link
Owner

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
Copy link
Owner

  • 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
Copy link
Contributor Author

mkatychev commented Sep 12, 2024

  • 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 rust-lang/rust-clippy#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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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 #7

@mkatychev
Copy link
Contributor Author

Once #11 is reviewed, this can be closed.

@pchampin pchampin closed this as completed Oct 2, 2024
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

No branches or pull requests

2 participants