refactor: remove Matches/Diffs macro in cli#1820
Conversation
WalkthroughThe pull request refactors various CLI print modules by removing macros and type aliases related to Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1820 +/- ##
==========================================
+ Coverage 87.45% 87.47% +0.02%
==========================================
Files 96 96
Lines 15556 15565 +9
==========================================
+ Hits 13604 13615 +11
+ Misses 1952 1950 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/cli/src/print/json_print.rs (1)
321-322:Details
❓ Verification inconclusive
Consider the memory implications of collecting all matches.
While the changes to use concrete vector types improve code clarity and maintainability, collecting all matches into memory at once could potentially impact performance for large files with many matches. However, given that the results need to be serialized to JSON anyway, the memory trade-off is likely acceptable in this context.
Run this script to assess the typical number of matches in real-world usage:
Also applies to: 332-332, 341-341, 469-470, 529-530, 594-595
🏁 Script executed:
#!/bin/bash # Description: Check the average and maximum number of matches in test files # to understand memory implications. rg -U 'let matches = .+find_all.+collect' -A 1 | \ rg 'assert|expect' | \ rg -o '[0-9]+' || trueLength of output: 93
Memory Considerations Require Manual Verification
The code now clearly uses concrete vector types for collecting matches, which enhances clarity and maintainability. However, even though the serialized JSON output favors this approach, the script we ran to assess the typical match count produced no output. This inconclusive result means that you should manually verify that collecting all matches in memory does not negatively impact performance on large files.
- Please confirm through profiling or further testing that the number of matches in real-world datasets remains within acceptable limits.
- If extremely large files are expected, consider whether a streaming approach might be more efficient in future refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/cli/src/print/cloud_print.rs(4 hunks)crates/cli/src/print/colored_print.rs(11 hunks)crates/cli/src/print/colored_print/test.rs(7 hunks)crates/cli/src/print/interactive_print.rs(5 hunks)crates/cli/src/print/json_print.rs(14 hunks)crates/cli/src/print/mod.rs(2 hunks)crates/cli/src/run.rs(1 hunks)crates/cli/src/scan.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/cli/src/scan.rs
🧰 Additional context used
🧠 Learnings (1)
crates/cli/src/print/json_print.rs (1)
Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1500
File: crates/cli/src/config.rs:60-60
Timestamp: 2024-11-10T16:52:06.569Z
Learning: In the `ast-grep` codebase, it's acceptable for some callers of `find_rules` to intentionally ignore the `RuleStats` returned in its tuple `(RuleCollection<SgLang>, RuleStats)` when the statistics are not needed.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (11)
crates/cli/src/print/mod.rs (3)
23-23: LGTM! Type alias simplification improves clarity.The removal of the generic parameter
Land standardization toSgLangreduces complexity while maintaining functionality.
26-31: LGTM! Method signatures simplified to use concrete types.The transition from macro-based types to concrete
Vec<NodeMatch>andVec<Diff>improves readability and maintainability.Also applies to: 32-33
54-54: LGTM! Field type updated for consistency.The
node_matchfield type update aligns with the simplifiedNodeMatchtype alias.crates/cli/src/print/cloud_print.rs (2)
38-39: LGTM! Method signatures simplified to use concrete types.The transition from macro-based types to concrete
Vec<NodeMatch>andVec<Diff>improves readability and maintainability.Also applies to: 46-46, 50-50
134-134: LGTM! Test function updated to collect matches.The explicit collection of matches into a vector aligns with the new method signatures.
crates/cli/src/print/colored_print/test.rs (1)
51-51: LGTM! Test functions updated to collect matches.All test functions consistently updated to collect matches into vectors, aligning with the new method signatures while preserving test behavior.
Also applies to: 71-71, 111-111, 161-163, 178-178, 203-203, 272-272
crates/cli/src/print/interactive_print.rs (3)
67-68: LGTM! Method signatures simplified to use concrete types.The transition from macro-based types to concrete
Vec<NodeMatch>andVec<Diff>improves readability and maintainability.Also applies to: 90-90, 94-94
96-97: LGTM! Collection handling updated.The explicit collection of diffs into a vector aligns with the new method signatures.
186-189: LGTM! Helper function signature updated.The
print_matches_and_confirm_nextfunction signature updated to use concrete types, maintaining consistency with other changes.crates/cli/src/run.rs (1)
329-329: LGTM! The changes align with the refactoring objectives.The modifications to collect iterator results into vectors before passing them to printer methods are consistent with the broader effort to simplify type handling across the codebase.
Also applies to: 331-331
crates/cli/src/print/colored_print.rs (1)
313-314: LGTM! The changes preserve the match merging functionality.The modifications to use concrete vector types are well-integrated with the existing match merging logic that handles overlapping and adjacent matches. The functionality for merging matches that start or end on the same line is maintained while improving code clarity.
Also applies to: 372-373, 422-423
fix #1819
Summary by CodeRabbit
Refactor
Tests