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

clippy: dissallow some methods and types #1411

Merged
merged 6 commits into from
Feb 28, 2023
Merged

clippy: dissallow some methods and types #1411

merged 6 commits into from
Feb 28, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Feb 26, 2023

clippy has a very powerful feature where you can forbid certain macros, methods and types.

Please take a look at the list - perhaps there are more things you'd like to forbid?

Checklist

@emilk emilk added the 🧑‍💻 dev experience developer experience (excluding CI) label Feb 26, 2023
crates/re_analytics/src/pipeline_native.rs Outdated Show resolved Hide resolved
crates/re_analytics/src/pipeline_native.rs Outdated Show resolved Hide resolved
@emilk emilk requested a review from teh-cmc February 26, 2023 18:58
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. Can't think of any methods to forbid myself right now, but surely will come up

disallowed-macros = [
'dbg',

# TODO(emilk): consider forbidding these to encourage the use of proper log stream, and then explicitly allow legitimate uses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's keeping us from that so far? Too widespread direct use?

Copy link
Member Author

@emilk emilk Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - it is used in a lot of build scripts and tests. We can add clippy exceptions to those files but I didn't want to make this PR too big or controversial in one go

"std::env::temp_dir", # Use the tempdir crate instead

# Disabled because of https://github.com/rust-lang/rust-clippy/issues/10406
# "std::time::Instant::now", # use `instant` crate instead for wasm/web compatability
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit this before and would be thankful for the clippy warning. Why can't we enable this warning yet?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because instant::Instant is an alias of std::time::Instant on native: rust-lang/rust-clippy#10406 (comment)

Comment on lines +45 to +46
# Allow-list of words for markdown in dosctrings https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
doc-valid-idents = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hit this so often <3

@emilk emilk merged commit 41034e8 into main Feb 28, 2023
@emilk emilk deleted the emilk/clippy-dissallow branch February 28, 2023 08:07
emilk added a commit that referenced this pull request Mar 2, 2023
* Allow-list some words in our markdown docstrings

* Forbid a bunch of macros, functions and types

* name the threads

* Do not ignore errors

* Replace use of std::sync::Mutex with parking_lot::Mutex

* Warn on failure to spawn analytics threads instead of panicing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants