refactor(token_usage): move functions to TokenUsageProvider directly#330
Merged
Conversation
|
Please add a description to this PR explaining what it does and why. See our coding conventions. |
alexsnaps
force-pushed
the
token-refactor
branch
2 times, most recently
from
July 10, 2026 14:01
a51f2bf to
d902ac9
Compare
praxis-bot
reviewed
Jul 13, 2026
praxis-bot
left a comment
Collaborator
There was a problem hiding this comment.
Review: refactor(token_usage): move functions to TokenUsageProvider directly
Clean mechanical refactoring. Reviewed all four changed files in full plus the unchanged consumer filters/src/token_count/tests.rs to verify no implicit Copy/Clone dependencies were missed.
What was checked
- Correctness:
match *selfcorrectly dereferences&Selffor enum pattern matching. All callers updated from free-function syntax to method syntax. Thepub(super)visibility onparse_anthropic_event/parse_bedrock_eventis the minimum needed for theimpl TokenUsageProviderblock in the parent module. - Caller completeness: Searched all code references to
extract_token_usage,extract_streaming_tokens, andTokenUsageProvideracross the repo. Every code-level caller is covered by the diff. The only remaining references to the old free functions are indocs/proposals/design documents (not compiled). - Implicit
Copyconsumers:filters/src/token_count/tests.rs(not in the diff) passesTokenUsageProviderby value intomake_filter()and therun_*_extractionhelpers. These are moves, not copies, so they compile withoutCopy. Confirmed by all 17 CI checks passing. - API surface: Removing
Clone,Copy, and the two public free functions is a breaking change, but appropriate for a 0.1.0 crate where all consumers are workspace-internal. - Test coverage: The test changes are purely syntactic (free function calls to method calls). No tests were added or removed, which is correct since no behavior changed.
- Convention compliance: File ordering, section separators, doc comments, visibility, and import grouping all conform to workspace conventions.
Severity table
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 0 |
No findings. The refactoring is correct, complete, and consistent.
Signed-off-by: Alex Snaps <alex@wcgw.dev>
alexsnaps
force-pushed
the
token-refactor
branch
from
July 15, 2026 12:33
d902ac9 to
c8a76f4
Compare
shaneutt
approved these changes
Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See #284
But, tl;dr, there is no need to have
TokenUsageProviderbeCloneorCopy(and copy it all over the place). This closes the door to eventually add state seamlessly in the future. Finally, both the methods where methods all along, so made them that, using&self