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.

Sign up for GitHub

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

Jump to bottom

Add clippy lints #176

Merged
merged 3 commits into from
Jan 8, 2024
Merged

Add clippy lints #176

merged 3 commits into from
Jan 8, 2024

Conversation

TimonPost
Copy link
Member

@TimonPost TimonPost commented Jan 8, 2024 •
edited
Loading

Description of Changes

Copied all the up-to-date lints we use at embark and put them into the workspace toml such that they apply for every project. Also added lots of documentation to public types because I added a lint for this too.

  • Defines clippylints in workspace root
  • Fixes clippy suggestions
  • Makes documentation required for public types inside puffin.
  • Makes parkinglot always a dependency (can revert if this is not desired)

@TimonPost TimonPost changed the title Clippy suggestions Add clippy lints Jan 8, 2024
@TimonPost TimonPost marked this pull request as ready for review January 8, 2024 14:40
@TimonPost TimonPost requested a review from emilk as a code owner January 8, 2024 14:40
@TimonPost TimonPost requested review from jnises and removed request for emilk January 8, 2024 14:42
emilk
emilk approved these changes Jan 8, 2024
View reviewed changes
Copy link
Collaborator

@emilk emilk left a comment

Choose a reason for hiding this comment

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

I love it!

Are we sure this doesn't require updating the MSRV of the crate itself?

puffin/src/lib.rs
#[must_use]
pub fn register_scope(
pub fn register_named_scope(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change that slipped in – was that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

this function should be hidden from API as it should not be called by users. I will revert it in this PR but follow up with general encapsulation PR

@TimonPost TimonPost removed the request for review from jnises January 8, 2024 15:09
@mergify mergify bot merged commit 6aa58d6 into main Jan 8, 2024
6 of 7 checks passed
@mergify mergify bot deleted the timon/clippy-pass branch January 8, 2024 15:11
@TimonPost
Copy link
Member Author

TimonPost commented Jan 8, 2024 •
edited
Loading

Oh was not suposed to merge automatically. Maybe can disable this mergify bot. Or fine tune it to not merge when comments are still unresolved. I'll followup. Why do you think it requires updating the MSRV of the crate itself?

@emilk
Copy link
Collaborator

emilk commented Jan 8, 2024

Yeah, annoying bot.

Why do you think it requires updating the MSRV of the crate itself?

It shouldn't - I just now realized the new sections ([workspace.lints.rust]) are in the workspace toml, so it should not at all affect the crates.

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

@emilk emilk emilk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TimonPost @emilk