Conversation
When jumping from a qualified call like Module1.create() or Foo::bar(), extract the qualifier from the left context and re-rank results so that files whose path contains the qualifier appear first. This is safe by default: no results are removed, only reordered, and when no qualifier is present the behavior is unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughExtracts a qualifier from the left context (e.g., module or namespace), threads that qualifier through result-processing, boosts results whose paths contain the qualifier, and exposes the qualifier in xref results for qualifier-aware filtering, sorting, and jumping. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/dumb-jump-test.el (1)
1421-1433: Consider adding test for range operator exclusion.The PR objectives mention testing "range exclusion" for the
..operator (e.g., Ruby/Elixir ranges like1..10), but no such test case is present. Adding a test would verify this edge case.💡 Suggested additional test case
;; Add after line 1427: ;; Range operator ".." should not be parsed as qualifier (should (null (dumb-jump--extract-qualifier '(:left "1.." :right "10"))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dumb-jump-test.el` around lines 1421 - 1433, Add a test case to dumb-jump--extract-qualifier to ensure the range operator ".." is not treated as a qualifier: update the existing ert-deftest dumb-jump--extract-qualifier-test by inserting a check that (dumb-jump--extract-qualifier '(:left "1.." :right "10")) returns nil (or using (should (null ...))). This targets the function dumb-jump--extract-qualifier and verifies the range operator exclusion edge case referenced in the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dumb-jump.el`:
- Around line 4525-4528: The qualifier boost is applied too early to
match-no-comments via dumb-jump--boost-by-qualifier (wrapping the
dumb-jump-filter-no-start-comments(match-sorted, qualifier)), but later stages
(match-cur-file-front, re-sorts by path/line, and var-to-jump building) undo it;
instead, apply dumb-jump--boost-by-qualifier after the final ordering stage:
remove the current boost call around match-no-comments and call
dumb-jump--boost-by-qualifier on the already-ordered sequence produced by
match-cur-file-front (and ensure the top-level variable matches is constructed
from that boosted order when qualifier is non-nil) so that qualifier-promoted
candidates persist through subsequent re-sorting and when building var-to-jump.
In `@test/dumb-jump-test.el`:
- Around line 1428-1433: The test uses string= to compare the result of
dumb-jump--extract-qualifier against nil, which will error because string=
expects strings; change those assertions to check for nil safely (e.g. use
(should (null (dumb-jump--extract-qualifier ...))) or (should-not
(dumb-jump--extract-qualifier ...))). Update the three failing expectations that
call dumb-jump--extract-qualifier in the test to use null/not instead of string=
so nil results are handled without raising an error.
---
Nitpick comments:
In `@test/dumb-jump-test.el`:
- Around line 1421-1433: Add a test case to dumb-jump--extract-qualifier to
ensure the range operator ".." is not treated as a qualifier: update the
existing ert-deftest dumb-jump--extract-qualifier-test by inserting a check that
(dumb-jump--extract-qualifier '(:left "1.." :right "10")) returns nil (or using
(should (null ...))). This targets the function dumb-jump--extract-qualifier and
verifies the range operator exclusion edge case referenced in the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50944f3e-39cf-4db0-8f22-426cf2d58a36
📒 Files selected for processing (2)
dumb-jump.eltest/dumb-jump-test.el
Apply dumb-jump--boost-by-qualifier after match-cur-file-front instead of at match-no-comments, so the boost isn't undone by the subsequent seq-sort on external results. Also use (null ...) instead of (string= ... nil) in tests for correctness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
Addressed both review comments in a41a83c:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dumb-jump.el (1)
4579-4606:⚠️ Potential issue | 🟠 MajorReturn the qualifier-boosted ordering to callers.
Line 4579 computes
match-qualified, but Line 4606 still returnsmatch-cur-file-frontand:resultsstays unboosted.dumb-jump-handle-resultsandxref-backend-definitionsboth read those fields, so non-aggressive jumps still present the old ordering even when a qualifier is present.Suggested fix
- (match-cur-file-front + (match-cur-file-ordered (if (not prefer-external) (append (seq-filter (lambda (it) (and (> (plist-get it :diff) 0) (or (string= (plist-get it :path) cur-file) (string= (plist-get it :path) rel-cur-file)))) match-no-comments) (seq-filter (lambda (it) (and (<= (plist-get it :diff) 0) (or (string= (plist-get it :path) cur-file) (string= (plist-get it :path) rel-cur-file)))) match-no-comments) (seq-sort #'dumb-jump--candidate-x<y (seq-filter (lambda (it) (not (or (string= (plist-get it :path) cur-file) (string= (plist-get it :path) rel-cur-file)))) match-no-comments))) (append (seq-sort #'dumb-jump--candidate-x<y (seq-filter (lambda (it) (not (or (string= (plist-get it :path) cur-file) (string= (plist-get it :path) rel-cur-file)))) match-no-comments)) (seq-filter (lambda (it) (or (string= (plist-get it :path) cur-file) (string= (plist-get it :path) rel-cur-file))) match-no-comments)))) - - (match-qualified - (dumb-jump--boost-by-qualifier match-cur-file-front qualifier)) + (match-cur-file-front + (dumb-jump--boost-by-qualifier match-cur-file-ordered qualifier)) (matches (if (not prefer-external) (seq-uniq (append - (dumb-jump-current-file-results cur-file match-qualified) - (dumb-jump-current-file-results rel-cur-file match-qualified))) - match-qualified)) + (dumb-jump-current-file-results cur-file match-cur-file-front) + (dumb-jump-current-file-results rel-cur-file match-cur-file-front))) + match-cur-file-front)) - (list :results results + (list :results match-cur-file-front :do-var-jump do-var-jump :var-to-jump var-to-jump :match-cur-file-front match-cur-file-front)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumb-jump.el` around lines 4579 - 4606, The code computes match-qualified (the qualifier-boosted ordering) but still returns :results and :match-cur-file-front using the unboosted match-cur-file-front; update the final plist so callers get the boosted ordering by returning match-qualified for :results and :match-cur-file-front (leave var-to-jump and do-var-jump logic that uses matches intact), ensuring dumb-jump-handle-results and xref-backend-definitions consume the qualifier-boosted ordering.
🧹 Nitpick comments (2)
test/dumb-jump-test.el (2)
1421-1430: Add the..negative case to the qualifier tests.This covers the happy paths, but it misses the regression the helper is specifically meant to avoid: treating a range operator as a qualifier separator. Please add one explicit
..fixture that must returnnil, otherwise that branch can regress without tripping this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dumb-jump-test.el` around lines 1421 - 1430, The test suite for dumb-jump--extract-qualifier is missing a negative case for the range operator ".."; add an explicit test assertion in dumb-jump--extract-qualifier-test that passes a left string containing ".." (e.g. '(:left "a..b" :right "()") or similar) and asserts the result is nil so the function does not treat ".." as a qualifier separator; update the test alongside the other should (null ...) cases referencing dumb-jump--extract-qualifier to prevent regressions.
1432-1445: Broaden this to verify the actual boost contract.Right now this only proves a single exact-case match moves to the front.
dumb-jump--boost-by-qualifieris meant to be case-insensitive and stable, so add at least one mixed-case qualifier/path pair and a second matching result to lock in both behaviors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dumb-jump-test.el` around lines 1432 - 1445, Update the dumb-jump--boost-by-qualifier test to assert the full boost contract: add at least one additional result whose :path matches the qualifier in a different case (e.g., "Module1" vs "module1") and another matching result so you can assert case-insensitivity and stability; after calling dumb-jump--boost-by-qualifier with qualifier "module1" assert the first N items are the matching entries (both the exact-case and mixed-case paths) and that their relative order is preserved, while non-matching entries remain after them; keep the existing nil-qualifier identity assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dumb-jump.el`:
- Around line 4579-4606: The code computes match-qualified (the
qualifier-boosted ordering) but still returns :results and :match-cur-file-front
using the unboosted match-cur-file-front; update the final plist so callers get
the boosted ordering by returning match-qualified for :results and
:match-cur-file-front (leave var-to-jump and do-var-jump logic that uses matches
intact), ensuring dumb-jump-handle-results and xref-backend-definitions consume
the qualifier-boosted ordering.
---
Nitpick comments:
In `@test/dumb-jump-test.el`:
- Around line 1421-1430: The test suite for dumb-jump--extract-qualifier is
missing a negative case for the range operator ".."; add an explicit test
assertion in dumb-jump--extract-qualifier-test that passes a left string
containing ".." (e.g. '(:left "a..b" :right "()") or similar) and asserts the
result is nil so the function does not treat ".." as a qualifier separator;
update the test alongside the other should (null ...) cases referencing
dumb-jump--extract-qualifier to prevent regressions.
- Around line 1432-1445: Update the dumb-jump--boost-by-qualifier test to assert
the full boost contract: add at least one additional result whose :path matches
the qualifier in a different case (e.g., "Module1" vs "module1") and another
matching result so you can assert case-insensitivity and stability; after
calling dumb-jump--boost-by-qualifier with qualifier "module1" assert the first
N items are the matching entries (both the exact-case and mixed-case paths) and
that their relative order is preserved, while non-matching entries remain after
them; keep the existing nil-qualifier identity assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd730b7a-0dd9-41f8-93de-9578903f16f6
📒 Files selected for processing (2)
dumb-jump.eltest/dumb-jump-test.el
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
1 participant
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.
Summary
Closes #409.
Module1.create()orFoo::bar(), extracts the qualifier (Module1/Foo) from the left context around the symbolHow it works
dumb-jump--extract-qualifierpulls the qualifier from the already-computed left context (handles.and::separators, excludes..range operators)dumb-jump--boost-by-qualifierdoes a stable partition of results — matching paths first, others after, preserving relative order within each groupdumb-jump-fetch-results→dumb-jump-process-resultsvia the existing plist plumbingWhy file-path matching works
In most languages, module/class names correlate with file names: Elixir
Module1→module1.ex, Pythonmodule1→module1.py, JavaModule1→Module1.java, RubyModule1→module1.rb, etc.Test plan
dumb-jump--extract-qualifier(dot, double-colon, range exclusion, nil/empty)dumb-jump--boost-by-qualifier(matching paths sorted first, nil is identity)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests