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

Add more automated checks for our code style #221

Closed
jplatte opened this issue Aug 15, 2020 · 17 comments · Fixed by #735
Closed

Add more automated checks for our code style #221

jplatte opened this issue Aug 15, 2020 · 17 comments · Fixed by #735
Assignees

Comments

@jplatte
Copy link
Member

jplatte commented Aug 15, 2020

We have had accidentally-private fields a bunch of times now, also mod.rs files have appeared a number of times after we initially moved away from them. I'd also like to enforce some other things, like how serde_json::Value should always be import-renamed to JsonValue. There might be existing tooling for some of this, but maybe it's also feasible to create new tooling for a few things.

@jplatte
Copy link
Member Author

jplatte commented Aug 17, 2020

Trying to create an exhaustive list here:

@iinuwa
Copy link
Member

iinuwa commented Aug 20, 2020

Would shell scripts to execute in CI suffice for (some of) these, or are you wanting to do something more robust?

(Reminds me of semgrep.)

@jplatte
Copy link
Member Author

jplatte commented Aug 20, 2020

For stuff like finding mod.rs, shell code would be fine. For anything that is about the insides of a Rust source file I'd rather have tools can actually parse Rust. I think most of the ground work for this kind of analysis has been done with rerast and rust-analyzer's structural search & replace. We may also be able to leverage rustfmt 2.0 in some way.

@agraven
Copy link
Contributor

agraven commented Aug 21, 2020

Maybe JsonValue could be better signposted as the proper way to do things by having a pub use with it somewhere, maybe in ruma-common?

@jplatte
Copy link
Member Author

jplatte commented Aug 21, 2020

And have other crates import it from there rather than serde_json? I kind of like having it obvious where it actually comes from. But it's certainly worth considering.

@jplatte
Copy link
Member Author

jplatte commented Oct 3, 2020

I've checked off the check for private fields – this is now checked for all types that derive Outgoing, which isn't a perfect solution but makes it much less important to have a dedicated lint :)

@jplatte
Copy link
Member Author

jplatte commented Feb 25, 2021

@DevinR528

This comment has been minimized.

@jplatte

This comment has been minimized.

@DevinR528

This comment has been minimized.

@jplatte

This comment has been minimized.

@DevinR528

This comment has been minimized.

@DevinR528

This comment has been minimized.

@DevinR528
Copy link
Member

DevinR528 commented Jun 1, 2021

PR rust-lang/rust-clippy#7299 for issue 7278

PR rust-lang/rust-clippy#7300 for issue 7276

RR rust-lang/rust-clippy#7315 for issue 7315 (merged)

@DevinR528
Copy link
Member

DevinR528 commented Jul 2, 2021

PR rust-lang/rust#86809 which will make non_exhaustive much nicer IMHO.

Major Change Proposal rust-lang/compiler-team#445 (comment)

@DevinR528
Copy link
Member

PR rust-lang/rust-clippy#7543 for the mod.rs ban.

@DevinR528
Copy link
Member

🎉

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

Successfully merging a pull request may close this issue.

4 participants