fix(libdd-trace-obfuscation): cargo clippy fix with all lints#1947
fix(libdd-trace-obfuscation): cargo clippy fix with all lints#1947Eldolfin wants to merge 25 commits into
Conversation
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1947 +/- ##
==========================================
- Coverage 72.69% 72.69% -0.01%
==========================================
Files 452 452
Lines 74886 74876 -10
==========================================
- Hits 54438 54428 -10
Misses 20448 20448
🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: cdc145d | Docs | Datadog PR Page | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Eldolfin
force-pushed
the
oscarld/restrictive-clippy-on-trace-obfuscation
branch
from
ca1e0f5 to
bb56e9b
Compare
| [package] | ||
| name = "libdd-trace-obfuscation" | ||
| version = "2.0.0" | ||
| version = "2.1.0" |
There was a problem hiding this comment.
Where did this come from? The current released version is 2.0.0. @iunanua any idea?
There was a problem hiding this comment.
Looking closer, adding the #[must_use] to fix the clippy::must_use_candidate attribute is considered a breaking change from the semver checks perspective.
Aaalibaba42
left a comment
Aaalibaba42
left a comment
There was a problem hiding this comment.
Overall this is great, glancing over the thing a little, I just don't like the ubiquitous 111_222_333 formatting given we are an ID heavy codebase, we rarely would have raw numbers above 999 compared to the numbers of times they would be IDs we don't want to format this way.
There was a problem hiding this comment.
I don't feel too comfortable stamping this 1k+ line change PR because it's a mix of trivial, semantic-preserving style changes (basically clippy --fix, as announced in the PR title) and two non trivial refactorings in the scanner and elsewhere (splitting huge functions into smaller ones). Would that be possible to split off those two refactorings first as two independent PRs (even it's just moving code around, it'll make it easier to make sure the changes are OK), and keep the final PR to be only the clippy-fix-like easy to review changes?
| /// Opcode returned by [`Scanner::step`] for each input char. | ||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| pub(crate) enum Op { | ||
| pub enum Op { |
There was a problem hiding this comment.
Why is this enum made public? It's changing the public API
| /// A streaming JSON scanner. Feed chars one at a time via [`Scanner::step`]; | ||
| /// the returned [`Op`] describes the structural significance of each char. | ||
| pub(crate) struct Scanner { | ||
| pub struct Scanner { |
There was a problem hiding this comment.
I think these are from this lint: redundant_pub_crate. Not a game changing lint, but the (crate) is indeed useless because this module is already private so it won't show up as public anyway.
@yannham I agree, this could have been split into 2 PRs, I can still relatively easily split it now because this refactor is mostly contained in a single commit (bb56e9b). On the other hand this crate is so heavily tested that I personally feel confortable merging this refactor without thinking too much about it (the full testsuite is not in this repo, but there is 303 sql test cases and 27 json obfuscation test cases accros different configs to give an idea). Lmk if you still think it's important do split it knowing that. Anyway, I'll try to go in smaller steps for crates I'm not as familiar with 👍🏽 BTW: the too_many_lines lint can be configured to change the default threshold (default is 100 lines). I feel like the default is already reasonable but lmk if you think otherwise ! |
|
I don't want to block this PR on splitting, and there are already a bunch of approves. However it's always good to get a second pair of eyes on the code, so even when you're confident on the change, we should strive to make reviews easy (or it kinda defeats the purpose to review in the first place). Let this be a reminder for our future selfs 🙂 |
note that it's weird that these don't appear anymore on stable. Maybe they were moved to another group or their logic was changed...
6 participants
What does this PR do?
Applies a restrictive workspace Clippy/Rust lint config and opts in
libdd-trace-obfuscation, fixing the resulting crate-local lints without changing the behavior.Motivation
Tighten lint coverage incrementally by starting with a single crate.
Additional Notes
How to test the change?
CI passing is sufficient validation.