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

ref(api): Only allow dead code where needed #2213

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

szokeasaurusrex
Copy link
Member

We added a blanket allow(dead_code) for the entire api module in #2096 because of some new clippy lints which were failing.

Here, we remove the blanket allow(dead_code) and only allow dead code for the specific lines where it occurs. It might not be possible to remove these because serde might rely on their presence.

We added a blanket `allow(dead_code)` for the entire api module in #2096 because of some new clippy lints which were failing.

Here, we remove the blanket `allow(dead_code)` and only allow dead code for the specific lines where it occurs. It might not be possible to remove these because serde might rely on their presence.
@loewenheim
Copy link
Contributor

AFAICT all of these types are only Deserialize. In that case it should be fine to remove the unused fields. serde will happily deserialize types from inputs that have "too many" fields, unless you use deny_unknown_fields.

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7b4171977ebcc5aa60503a6a1c6b87c1

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 5, 2024 14:07
@loewenheim
Copy link
Contributor

Also, if you do want to keep them, it might make sense to use #[expect(dead_code)] instead of #[allow(dead_code)], which was added a few versions ago. The difference is that with expect there is a warning if the lint doesn't trigger. This makes sure the field actually isn't used.

@szokeasaurusrex
Copy link
Member Author

@loewenheim let's keep the unused fields in case removing them has some unintended side effects. Keeping them around is lower risk than removing them.

I applied your suggestion to use expect rather than allow. Might be worth changing this throughout the codebase.

@loewenheim
Copy link
Contributor

I applied your suggestion to use expect rather than allow. Might be worth changing this throughout the codebase.

I agree, I think in most cases where we used to use allow we actually want expect, but it didn't exist.

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 5, 2024 14:20
@szokeasaurusrex szokeasaurusrex merged commit de07638 into master Nov 5, 2024
14 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/no-dead-code-api branch November 5, 2024 14:21
szokeasaurusrex added a commit that referenced this pull request Nov 6, 2024
This dead code caused a CI linter failure in `master`. Likely, we did not update #2212 with the changes from #2213 before merging.
szokeasaurusrex added a commit that referenced this pull request Nov 6, 2024
This dead code caused a CI linter failure in `master`. Likely, we did
not update #2212 with the changes from #2213 before merging.
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

Successfully merging this pull request may close these issues.

2 participants