-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
[Merged by Bors] - Restructure lint lists in boa_ast
#2433
Conversation
Test262 conformance changes
|
Codecov Report
@@ Coverage Diff @@
## main #2433 +/- ##
===========================================
- Coverage 52.41% 38.86% -13.55%
===========================================
Files 329 316 -13
Lines 34945 24099 -10846
===========================================
- Hits 18315 9367 -8948
+ Misses 16630 14732 -1898
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
The one lint I'd argue that should be warn is The other lints I don't have a problem with, but let's see what the others say :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I agree with @jedel1043 about missing_docs
being warn, I't can be quite bothersome.
Besides that this looks good :)
EDIT: Having a CI task that checks this (and other annoying to prototype lints) would be nice, that way we enforce only documented code to be pushed to main
branch, what do you think?
I'm OK to put |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change :) this looks very good. We should probably apply it to all the crates in the engine.
Also, as I mentioned, I would deny warnings in the CI.
To deny warnings in CI I think we just need to add the I definitely feel that this should be simplified, having 50 lines selecting which lints to allow or deny copied among all our crates isn't very clean. Looking at the clippy documentation it says that configuring clippy through a |
We would need to copy the same configuration for now, and add the RUSTFLAGS, yes |
I think coppying the lint list is okay for now. I will add |
+1 to this |
I don't believe there is any alternative @jasonwilliams and it's not that they are necessarily going to deprecate it but they say that they might do so in the future. |
Do we want to merge this as is based on the discussion? |
Bors r+ |
This Pull Request restructures the lint deny/warn/allow lists in `boa_ast` and fixes some clippy lints in the crate. The most relevant change should be in `boa_ast/src/lib.rs`. I went trough all the lints that are available in rustc/rustdoc/clippy with the goal to have most in the deny list, either trough groups or individually. Some clippy lints remain allowed, because they trigger false positives (I fixed them as far as possible). I'm interested in how everyone feels about denying most lints, as it may impact individual development workflows. If we agree on how we want to structure this, I will apply the restructured lists to all other creates aswell.
Pull request successfully merged into main. Build succeeded: |
boa_ast
boa_ast
This Pull Request restructures the lint deny/warn/allow lists in
boa_ast
and fixes some clippy lints in the crate.The most relevant change should be in
boa_ast/src/lib.rs
. I went trough all the lints that are available in rustc/rustdoc/clippy with the goal to have most in the deny list, either trough groups or individually. Some clippy lints remain allowed, because they trigger false positives (I fixed them as far as possible).I'm interested in how everyone feels about denying most lints, as it may impact individual development workflows. If we agree on how we want to structure this, I will apply the restructured lists to all other creates aswell.