apollographql / router

A configurable, high-performance routing runtime for Apollo Federation 🚀
https://www.apollographql.com/docs/router/
Other
800 stars 268 forks source link

Add deny lints to QP #6061

Open TylerBloom opened 3 days ago

TylerBloom commented 3 days ago

This PR changes the crate-wide #[allow(dead_code)] lint directive to deny and denys a few other lints.


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

[^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

github-actions[bot] commented 3 days ago

@TylerBloom, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

router-perf[bot] commented 3 days ago

CI performance tests

TylerBloom commented 2 days ago

I don't think we should deny unused code as it would prevent compilation if you're eg. commenting something out while working on a bug. It should be a warning.

My thought about denying these lints is that changing deny to allow/warn is easy to do while debugging or writing a feature. When the PR is ready, that can be changed back. However, we shouldn't merge a PR that has dead code, anddeny is the easiest way to prevent that.

goto-bus-stop commented 1 day ago

We shouldn't merge a PR with warnings 😛

In apollo-rs we run the linter with -D warnings in CI, I guess we don't do that here. Anyways, it's just my personal preference, we don't have to do it 🙂 Nvm, we do do that here, so PRs with warnings already cannot get merged.