Added linter for warning about unnecessary usage of ^ in code #754
Conversation
Signed-off-by: SNEHA KB <[email protected]>
Signed-off-by: SNEHA KB <[email protected]>
Added lint file to detect redundant ^
WalkthroughIntroduces a new lint LtRedundantRho that scans EO XMIR for redundant ξ.ρ usages and emits WARNING defects when the caret is unnecessary. Adds a corresponding JUnit 5 test suite validating rule naming, no-defect scenarios, and defect reporting with multiple matches. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Lint as LtRedundantRho
participant XMIR as XMIR XML
participant Nav as Xnav View
participant Rep as Defect Reporter
Dev->>Lint: defects(XMIR)
Lint->>XMIR: Load XML
Lint->>Nav: Build navigation view
Nav-->>Lint: Objects with base starting ξ.ρ
loop For each candidate object
Lint->>Nav: Resolve object name and ancestors
alt Exactly one match
Lint->>Rep: WARNING: redundant 'ξ.ρ' (single match)
else Multiple matches and ancestor target matches
Lint->>Rep: WARNING: redundant 'ξ.ρ' (resolves same)
else Necessary usage
Lint-->>Dev: No defect
end
end
Rep-->>Dev: Collected defects
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Signed-off-by: SNEHA KB <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/test/java/org/eolang/lints/LtRedundantHatTest.java (3)
22-34: Test doesn’t exercise “necessary hat” scenario; it only has no^at all.This currently asserts “no defects” because there are zero
o[@base='^']nodes, not because a hat is necessary. Either rename the test to reflect that or construct XMIR that actually contains a necessary^and assert no defects.I can help craft realistic XMIR for both “necessary hat” and “redundant hat” cases once we lock the XPath used in the lint.
36-44: Rename to present tense to satisfy qulice (RulePresentTense).Apply:
- void motiveContainsGuidance() throws IOException { + void containsGuidanceInMotive() throws IOException {@@ - @Test - void motiveContainsGuidance() throws IOException { + @Test + void containsGuidanceInMotive() throws IOException {
46-54: Rename to present tense to satisfy qulice (RulePresentTense).Apply:
- void nameIsStableId() { + void returnsStableId() {@@ - @Test - void nameIsStableId() { + @Test + void returnsStableId() {src/main/java/org/eolang/lints/LtRedundantHat.java (1)
51-53: Motive string grammar.Replace “please omitted” with “please omit it”.
- return "The '^' notation is used when there are no naming conflicts, please omitted for brevity."; + return "The '^' notation should be used only when there are naming conflicts; otherwise, please omit it for brevity.";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/eolang/lints/LtRedundantHat.java(1 hunks)src/test/java/org/eolang/lints/LtRedundantHatTest.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: qulice
src/test/java/org/eolang/lints/LtRedundantHatTest.java
[error] 1-1: Test name 'nameIsStableId' doesn't follow present-tense naming convention. Rename to present tense. (RulePresentTense)
[error] 1-1: Test name 'motiveContainsGuidance' doesn't follow present-tense naming convention. Rename to present tense. (RulePresentTense)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: deep
- GitHub Check: ort
🔇 Additional comments (2)
src/main/java/org/eolang/lints/LtRedundantHat.java (2)
21-21: Access level check.If this lint must be discoverable outside the package (e.g., via service loading), make the class public; otherwise package‑private is fine. Please confirm intended visibility.
27-46: Fix inverted necessity check and make hat-name extraction null-safeFile: src/main/java/org/eolang/lints/LtRedundantHat.java
Lines: 27-46
- Wrong necessity check: current logic looks for any ancestor that contains a descendant with the same name (inverted). Only the nearest container (ancestor::o[1]) should be checked for a local sibling; a hat is necessary only when the nearest container already defines the same name.
- Robustness: object.attribute("name").text().get() throws when @name is absent on the hat; derive the name safely from hat/@name or the first child o[@name] and skip if unknown to avoid false positives.
Apply:
- final List<Xnav> objects = xml.path("//o[@base='^']").collect(Collectors.toList()); - for (final Xnav object : objects) { - final String name = object.attribute("name").text().get(); - final List<Xnav> parents = object.path(String.format("ancestor::o[.//o[@name='%s']]", name)) - .collect(Collectors.toList()); - if (parents.isEmpty()) { + final List<Xnav> hats = xml.path("//o[@base='^']").collect(Collectors.toList()); + for (final Xnav hat : hats) { + String name = hat.attribute("name").text().orElse(null); + if (name == null || name.isEmpty()) { + final List<Xnav> named = hat.path("o[@name]").collect(Collectors.toList()); + if (named.isEmpty()) { + // Can't reliably determine the target; skip to avoid false positives + continue; + } + name = named.get(0).attribute("name").text().orElse(null); + if (name == null || name.isEmpty()) { + continue; + } + } + // A hat is necessary only if the nearest container already defines the same name. + final List<Xnav> locals = hat + .path(String.format("ancestor::o[1]/o[@name='%s']", name)) + .collect(Collectors.toList()); + if (locals.isEmpty()) { defects.add( new Defect.Default( this.name(), Severity.WARNING, - new OnDefault(xmir).get(), - Integer.parseInt(object.attribute("line").text().orElse("0")), + new OnDefault(xmir).get(), + Integer.parseInt(hat.attribute("line").text().orElse("0")), String.format( "Redundant '^' notation: '%s' can be accessed without it", name ) ) ); } }Repository search produced no matches for o[@base='^'] in the earlier run (empty output). Re-run these checks from the repo root and paste outputs:
rg -nP -C3 --glob '/*.xml' 'base="^"'
rg -nP --glob '/*.xml' '<o[^>]*base="^"[^>]*name="' -c
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/test/java/org/eolang/lints/LtRedundantHatTest.java (3)
25-25: Fix Checkstyle cascade indentation (line 25)Indent the continued argument by +4 (or align with the opening parenthesis). Example:
- final XML xml = new XMLDocument( - "<program><object name='foo' line='10'><o name='+test' line='12'></o></object></program>" - ); + final XML xml = new XMLDocument( + "<program><object name='foo' line='10'><o name='+test' line='12'></o></object></program>" + );
30-30: Fix Checkstyle cascade indentation (line 30)Arguments of assertThat should be indented consistently:
- MatcherAssert.assertThat( - "Should not report a defect when '^' is necessary to resolve ambiguity", - defects, - Matchers.empty() - ); + MatcherAssert.assertThat( + "Should not report a defect when '^' is necessary to resolve ambiguity", + defects, + Matchers.empty() + );
7-13: Remove unused IOException import and throws clausesNeither test performs I/O. Drop the import and the throws in methods to keep qulice happy.
-import java.io.IOException;- void reportsNoDefectsForNecessaryHat() throws IOException { + void reportsNoDefectsForNecessaryHat() {- void containsGuidanceInMotive() throws IOException { + void containsGuidanceInMotive() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/org/eolang/lints/LtRedundantHatTest.java(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: qulice
src/test/java/org/eolang/lints/LtRedundantHatTest.java
[error] 25-25: Checkstyle: Indentation issue at LtRedundantHatTest.java:25. CascadeIndentationCheck violation.
[error] 30-30: Checkstyle: Indentation issue at LtRedundantHatTest.java:30. CascadeIndentationCheck violation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: deep
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (macos-15, 21)
🔇 Additional comments (1)
src/test/java/org/eolang/lints/LtRedundantHatTest.java (1)
47-54: Stable rule id assertion looks goodAsserting name() == "redundant-hat" is clear and stable.
Signed-off-by: SNEHA KB <[email protected]>
Signed-off-by: SNEHA KB <[email protected]>
Signed-off-by: SNEHA KB <[email protected]>
Signed-off-by: SNEHA KB <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/org/eolang/lints/LtRedundantHat.java (4)
49-68: Review ancestor comparison logic.The logic compares
match.equals(target)wheretargetisancestors.get(0). However, this comparison might not work as expected since it's comparingXnavobjects, and the equality semantics may not match what's intended (comparing XML nodes vs. object identity).Consider clarifying the comparison logic. If you need to compare XML nodes, you might need to compare their underlying DOM nodes or use a more specific comparison method:
- if (match.equals(target)) { + if (match.node().equals(target.node())) {Or verify that
Xnav.equals()has the correct semantics for this use case.
42-42: Potential NumberFormatException on malformed line attribute.The code uses
Integer.parseInt()without catching potentialNumberFormatExceptionif the line attribute contains non-numeric data.Add proper error handling:
- Integer.parseInt(obj.attribute("line").text().orElse("0")), + this.parseLineNumber(obj.attribute("line").text().orElse("0")),And add a helper method:
private int parseLineNumber(final String line) { try { return Integer.parseInt(line); } catch (final NumberFormatException ex) { return 0; } }Also applies to: 58-58
74-76: Consider more descriptive motive text.The motive could be more helpful by explaining when the
^notation is actually necessary vs. when it's redundant.- return "The '^' notation is redundant and can be omitted for brevity."; + return "The '^' notation is redundant when there's no name ambiguity and can be omitted for brevity.";
23-23: Consider using more appropriate initial capacity.The
ArrayListis initialized with capacity 0, which will cause immediate reallocation when defects are added. Since you're processing multiple objects, a small initial capacity would be more efficient.- final Collection<Defect> defects = new ArrayList<>(0); + final Collection<Defect> defects = new ArrayList<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/eolang/lints/LtRedundantHat.java(1 hunks)src/test/java/org/eolang/lints/LtRedundantHatTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/eolang/lints/LtRedundantHatTest.java
🔇 Additional comments (2)
src/main/java/org/eolang/lints/LtRedundantHat.java (2)
20-20: LGTM! Appropriate package-private visibility.The class is correctly scoped as package-private since it's an internal implementation detail of the linting framework.
36-48: Don't treat '^' as redundant just because matches.size() == 1.matches.size() == 1 can produce false positives — the hat (^) may be required for scoping/upward traversal even if only one object with that name exists globally; verify EO/XMIR semantics for base='^' and adjust the lint to confirm the caret doesn't change resolution (e.g., nearest-ancestor vs. global lookup) before reporting.
Location: src/main/java/org/eolang/lints/LtRedundantHat.java (around lines 36–48).
|
Hi @yegor256 Kindly review the PR and merge it. All workflows and checks have passed. Thank you! |
|
@h1alexbel could you please review this one? |
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@Snehakb1219-christ please check
| final Collection<Defect> defects = new ArrayList<>(0); | ||
| final Xnav xml = new Xnav(xmir.inner()); | ||
| final List<Xnav> objs = xml | ||
| .path("//o[@base='^']") |
There was a problem hiding this comment.
@Snehakb1219-christ in EO, we can't have @base="^", the correct representation of attribute is @base="ξ.ρ". After ξ.ρ we can call any other attribute, e.g.: ξ.ρ.foo.
| * Lint that warns if a redundant {@code ^} is used. | ||
| * @since 0.0.59 | ||
| */ | ||
| final class LtRedundantHat implements Lint<XML> { |
There was a problem hiding this comment.
@Snehakb1219-christ looks like we can implement this lint in XSL, there is no need to do it in Java, I believe
There was a problem hiding this comment.
@h1alexbel It works fine in java also. I think it's necessary to do this in XSL.
WDYT?
There was a problem hiding this comment.
@Snehakb1219-christ I believe that we should implement all lints in XSL. Java is for cases, where XSL won't work. Make sense?
Signed-off-by: SNEHA KB <[email protected]>
Signed-off-by: SNEHA KB <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/main/java/org/eolang/lints/LtRedundantRho.java (2)
26-26: Avoid pre-sizing with zero.
new ArrayList<>(0)is unnecessary and misleading; just use the diamond operator.- final Collection<Defect> defects = new ArrayList<>(0); + final Collection<Defect> defects = new ArrayList<>();
45-45: Safer line parsing.If
@lineis missing or malformed,Integer.parseIntthrows. Defaulting to 0 hides issues. Consider a safe parse helper with a non-zero fallback (e.g., element line or 1).- Integer.parseInt(obj.attribute("line").text().orElse("0")), + obj.attribute("line").text().map(Integer::parseInt).orElse(1),src/test/java/org/eolang/lints/LtRedundantRhoTest.java (3)
22-32: Use a realistic EO/XMIR sample and precise emptiness matcher.
- The EO snippet
^foolikely should be^.foo. Please confirm actual syntax to ensure the test exercises the rule.- Prefer
emptyIterable()for clarity overiterableWithSize(equalTo(0)).- new EoSyntax("[] > foo\n+test > @\n^foo > bar").parsed() + new EoSyntax("[] > foo\n+test > @\n^.foo > bar").parsed() @@ - Matchers.allOf( - Matchers.iterableWithSize(Matchers.equalTo(0)) - ) + Matchers.emptyIterable()
43-52: Duplicate test scenario; consolidate.
doesNotReportDefectWhenCaretIsNecessaryrepeats the first test with the same input/assertion. Merge into one parametrized test or drop the duplicate to keep the suite lean.
1-13: Consider adding a motive test.Add a simple check that
new LtRedundantRho().motive()returns non-empty text and loads without exceptions. This guards resource regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/eolang/lints/LtRedundantRho.java(1 hunks)src/test/java/org/eolang/lints/LtRedundantRhoTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: deep
🔇 Additional comments (1)
src/test/java/org/eolang/lints/LtRedundantRhoTest.java (1)
35-41: Rule id assertion LGTM.Ensures stability of the lint’s external identifier.
| final List<Xnav> objs = xml | ||
| .path("//o[starts-with(@base,'ξ.ρ')]") | ||
| .collect(Collectors.toList()); | ||
| for (final Xnav obj : objs) { |
There was a problem hiding this comment.
Don't match bare ξ.ρ; restrict to attribute-qualified uses.
Bare ^ (base=ξ.ρ) can be required; we only want ^.<attr>. Tighten the selector or guard in code to avoid false positives.
Apply either of the following:
Option A (selector):
- .path("//o[starts-with(@base,'ξ.ρ')]")
+ .path("//o[starts-with(@base,'ξ.ρ.')]")Option B (guard):
- for (final Xnav obj : objs) {
- final String name = obj.attribute("name").text().orElse("");
+ for (final Xnav obj : objs) {
+ final String base = obj.attribute("base").text().orElse("");
+ if ("ξ.ρ".equals(base) || !base.startsWith("ξ.ρ.")) {
+ continue; // ignore bare ^ or malformed cases
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final List<Xnav> objs = xml | |
| .path("//o[starts-with(@base,'ξ.ρ')]") | |
| .collect(Collectors.toList()); | |
| for (final Xnav obj : objs) { | |
| final List<Xnav> objs = xml | |
| .path("//o[starts-with(@base,'ξ.ρ.')]") | |
| .collect(Collectors.toList()); | |
| for (final Xnav obj : objs) { |
🤖 Prompt for AI Agents
In src/main/java/org/eolang/lints/LtRedundantRho.java around lines 28 to 31, the
XPath selector currently matches any element whose base starts with "ξ.ρ", which
also hits bare ξ.ρ; restrict matches to attribute-qualified uses by either
tightening the XPath to require the dot (e.g. starts-with(@base,'ξ.ρ.') or
matching a pattern that includes a dot/attribute) or keep the XPath but add a
guard inside the loop that only processes objs whose @base contains a '.' after
"ξ.ρ" (e.g. check that the base string startsWith "ξ.ρ." or contains "ξ.ρ.").
Implement one of these changes so bare ξ.ρ is ignored.
| final String name = obj.attribute("name").text().orElse(""); | ||
| final List<Xnav> matches = xml | ||
| .path(String.format("//o[@name='%s']", name)) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Wrong identifier extracted; use attribute after ξ.ρ. instead of @name.
@name is the alias of the current <o>, not the target attribute referenced via ^. This causes incorrect matching and messages.
Apply this diff to extract the correct attribute:
- final String name = obj.attribute("name").text().orElse("");
+ final String base = obj.attribute("base").text().orElse("");
+ final String attr = base.startsWith("ξ.ρ.") ? base.substring("ξ.ρ.".length()) : "";
+ if (attr.isEmpty()) {
+ continue;
+ }Then replace subsequent usages of name in this method with attr (messages and queries).
🤖 Prompt for AI Agents
In src/main/java/org/eolang/lints/LtRedundantRho.java around lines 32 to 35, the
code extracts the current <o>'s @name but should extract the referenced
attribute after the ξ.ρ. expression; change the extraction to read the attribute
name from the ξ.ρ. reference (save it to a variable named attr) and then replace
all subsequent uses of the previous name variable in this method (queries, match
collection and message text) with attr so queries target the referenced
attribute and messages report the correct identifier.
| final List<Xnav> ancestors = obj | ||
| .path(String.format("ancestor::o[@name='%s']", name)) | ||
| .collect(Collectors.toList()); | ||
| if (matches.size() == 1) { | ||
| defects.add( | ||
| new Defect.Default( | ||
| this.name(), | ||
| Severity.WARNING, | ||
| new OnDefault(xmir).get(), | ||
| Integer.parseInt(obj.attribute("line").text().orElse("0")), | ||
| String.format( | ||
| "Redundant 'ξ.ρ' notation: '%s' can be accessed without it", | ||
| name | ||
| ) | ||
| ) | ||
| ); | ||
| } else if (!ancestors.isEmpty() && matches.size() > 1) { | ||
| final Xnav target = ancestors.get(0); | ||
| for (final Xnav match : matches) { | ||
| if (match.equals(target)) { | ||
| defects.add( | ||
| new Defect.Default( | ||
| this.name(), | ||
| Severity.WARNING, | ||
| new OnDefault(xmir).get(), | ||
| Integer.parseInt(obj.attribute("line").text().orElse("0")), | ||
| String.format( | ||
| "Redundant 'ξ.ρ' notation: '%s' resolves to the same object without it", | ||
| name | ||
| ) | ||
| ) | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundancy check ignores local shadowing; produces false positives.
Current logic counts all @name across the whole document and compares to ancestors, which will warn even when a local definition shadows the parent (a case where ^ is necessary).
Use a scope-aware check: warn only if the immediate scope doesn’t define attr locally and a parent scope does. Example refactor:
- final List<Xnav> matches = xml
- .path(String.format("//o[@name='%s']", name))
- .collect(Collectors.toList());
- final List<Xnav> ancestors = obj
- .path(String.format("ancestor::o[@name='%s']", name))
- .collect(Collectors.toList());
- if (matches.size() == 1) {
- defects.add(
- new Defect.Default(
- this.name(),
- Severity.WARNING,
- new OnDefault(xmir).get(),
- Integer.parseInt(obj.attribute("line").text().orElse("0")),
- String.format(
- "Redundant 'ξ.ρ' notation: '%s' can be accessed without it",
- name
- )
- )
- );
- } else if (!ancestors.isEmpty() && matches.size() > 1) {
- final Xnav target = ancestors.get(0);
- for (final Xnav match : matches) {
- if (match.equals(target)) {
- defects.add(
- new Defect.Default(
- this.name(),
- Severity.WARNING,
- new OnDefault(xmir).get(),
- Integer.parseInt(obj.attribute("line").text().orElse("0")),
- String.format(
- "Redundant 'ξ.ρ' notation: '%s' resolves to the same object without it",
- name
- )
- )
- );
- break;
- }
- }
- }
+ // Is there a local definition in the current scope that would shadow the parent?
+ final boolean hasLocal =
+ !obj.path(String.format("ancestor::o[1]/o[@name='%s']", attr))
+ .collect(Collectors.toList()).isEmpty();
+ // Does the parent scope define the attribute?
+ final boolean parentHasAttr =
+ !obj.path(String.format("ancestor::o[2]/o[@name='%s']", attr))
+ .collect(Collectors.toList()).isEmpty();
+ if (!hasLocal && parentHasAttr) {
+ defects.add(
+ new Defect.Default(
+ this.name(),
+ Severity.WARNING,
+ new OnDefault(xmir).get(),
+ Integer.parseInt(obj.attribute("line").text().orElse("0")),
+ String.format(
+ "Redundant 'ξ.ρ' notation: '%s' can be accessed without it",
+ attr
+ )
+ )
+ );
+ }Note: If XMIR nesting differs in your project, adjust ancestor::o[1]/[2] accordingly. Add tests to cover shadowing and parent access.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/org/eolang/lints/LtRedundantRho.java around lines 36 to 71, the
current check counts all @name occurrences across the document and emits
warnings even when the attribute is shadowed locally; change the logic to be
scope-aware: first check the immediate scope (the current object's direct
children or attributes) for a local definition of the name and skip the warning
if present; only proceed to inspect the nearest parent scope (the closest
ancestor o node) and emit the redundant-ρ warning when the name is not defined
locally and it is defined in a parent scope (i.e., localMatches.isEmpty() &&
parentMatches.notEmpty()); implement this by selecting only same-scope attribute
nodes for the local check and only the nearest ancestor o for the parent check,
update the existing conditional branches to use these results, and add unit
tests covering local shadowing, parent access, and the no-parent case.
| return new UncheckedText( | ||
| new TextOf( | ||
| new ResourceOf( | ||
| String.format( | ||
| "org/eolang/motives/errors/%s.md", this.name() | ||
| ) | ||
| ) | ||
| ) | ||
| ).asString(); |
There was a problem hiding this comment.
🧩 Analysis chain
Resource lookup: verify motive path and existence.
Path hardcodes errors/. Ensure resources/org/eolang/motives/errors/redundant-hat.md exists and is packaged. Add a unit test for motive() returning non-empty.
🏁 Script executed:
#!/bin/bash
# Verify motive file exists and is included.
fd -a 'redundant-hat.md' resources | sed -n '1p'
rg -n 'redundant-hat' -g resources/org/eolang/motives/**
# Also check the lint is registered in any aggregator/factory.
rg -nP '(redundant-hat|LtRedundantRho)' -g 'src/main/java/**'Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "Searching for 'redundant-hat.md' and occurrences..."
rg -n --hidden --no-ignore -S 'redundant-hat.md' || true
rg -n --hidden --no-ignore -S 'redundant-hat' || true
echo
echo "Searching for motives/errors md files..."
rg -n --hidden --no-ignore -S 'org/eolang/motives/errors' || true
echo
echo "Listing src/main/resources MD files (if present)..."
if [ -d src/main/resources ]; then
find src/main/resources -type f -name '*.md' -print || true
else
echo "src/main/resources not found"
fi
echo
echo "Showing LtRedundantRho.java (first 240 lines)..."
sed -n '1,240p' src/main/java/org/eolang/lints/LtRedundantRho.java || true
echo
echo "Searching repo for motive() usages and tests referencing LtRedundantRho or redundant-hat..."
rg -n --hidden --no-ignore -S 'motive\s*\(|LtRedundantRho|redundant-hat' || trueLength of output: 15067
Create missing motive resource and add a motive test
motive() loads "org/eolang/motives/errors/redundant-hat.md" but that file is not present under src/main/resources/org/eolang/motives/errors/ — add src/main/resources/org/eolang/motives/errors/redundant-hat.md (or change the path in src/main/java/org/eolang/lints/LtRedundantRho.java). Ensure the resource is packaged, and add/extend src/test/java/org/eolang/lints/LtRedundantRhoTest.java to assert new LtRedundantRho().motive() returns a non-empty string.
🤖 Prompt for AI Agents
In src/main/java/org/eolang/lints/LtRedundantRho.java around lines 78 to 86 the
motive() call expects a resource at "org/eolang/motives/errors/redundant-hat.md"
which is missing; either add the resource file
src/main/resources/org/eolang/motives/errors/redundant-hat.md (with the
appropriate message content) and ensure it is included in the build resources,
or change the String.format path in this file to point to an existing resource;
additionally add or extend
src/test/java/org/eolang/lints/LtRedundantRhoTest.java to instantiate new
LtRedundantRho() and assert that motive() returns a non-empty string so the
resource loading is validated.
| void reportsDefectWhenCaretIsRedundantWithMultipleMatches() throws IOException { | ||
| MatcherAssert.assertThat( | ||
| "Should report a defect when '^' is redundant among multiple matches", | ||
| new LtRedundantRho().defects( | ||
| new XMLDocument("<object><o name='foo' base='ξ.ρ' line='12'></o></object>") | ||
| ), | ||
| Matchers.hasSize(1) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Test name/content mismatch; second branch not covered.
The test claims “multiple matches” but provides a single <o>; it only exercises the single‑match branch. Either rename the test or extend XML to include both a parent attribute and the ξ.ρ.<attr> usage.
Suggested test data to cover the intended case (adjust if your XMIR differs):
- new XMLDocument("<object><o name='foo' base='ξ.ρ' line='12'></o></object>")
+ new XMLDocument(
+ "<object>" +
+ "<o name='foo'/>" +
+ "<o name='inner'>" +
+ "<o name='bar' base='ξ.ρ.foo' line='12'/>" +
+ "</o>" +
+ "</object>"
+ )And add a negative test to prevent false positives when local shadowing exists:
@Test
void doesNotReportWhenLocalShadowingRequiresHat() throws IOException {
MatcherAssert.assertThat(
new LtRedundantRho().defects(
new XMLDocument(
"<object>" +
"<o name='foo'/>" +
"<o name='inner'>" +
"<o name='foo'/>" + // local shadow
"<o name='bar' base='ξ.ρ.foo' line='7'/>" +
"</o>" +
"</object>"
)
),
Matchers.emptyIterable()
);
}🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for LtRedundantRhoTest..."
rg -n --hidden -S "LtRedundantRhoTest" || true
echo "---- Showing file (if exists) ----"
sed -n '1,240p' src/test/java/org/eolang/lints/LtRedundantRhoTest.java || true
echo "---- Searching for related test names ----"
rg -n --hidden -S "reportsDefectWhenCaretIsRedundantWithMultipleMatches|doesNotReportWhenLocalShadowingRequiresHat|doesNotReport|reportsDefect" src/test/java/org/eolang/lints || trueLength of output: 2734
Test name/content mismatch — second branch not covered.
The test reportsDefectWhenCaretIsRedundantWithMultipleMatches (src/test/java/org/eolang/lints/LtRedundantRhoTest.java:55-62) supplies a single (base='ξ.ρ') and only exercises the single-match branch; rename the test or extend the XML to produce multiple matches.
Suggested change:
- new XMLDocument("<object><o name='foo' base='ξ.ρ' line='12'></o></object>")
+ new XMLDocument(
+ "<object>" +
+ "<o name='foo'/>" +
+ "<o name='inner'>" +
+ "<o name='bar' base='ξ.ρ.foo' line='12'/>" +
+ "</o>" +
+ "</object>"
+ )Add a negative test to avoid false positives with local shadowing:
@Test
void doesNotReportWhenLocalShadowingRequiresHat() throws IOException {
MatcherAssert.assertThat(
new LtRedundantRho().defects(
new XMLDocument(
"<object>" +
"<o name='foo'/>" +
"<o name='inner'>" +
"<o name='foo'/>" + // local shadow
"<o name='bar' base='ξ.ρ.foo' line='7'/>" +
"</o>" +
"</object>"
)
),
Matchers.emptyIterable()
);
}🤖 Prompt for AI Agents
In src/test/java/org/eolang/lints/LtRedundantRhoTest.java around lines 55-62,
the test named reportsDefectWhenCaretIsRedundantWithMultipleMatches only
supplies a single <o> so it covers the single-match branch; update the test to
actually produce multiple matches or add the suggested negative test to cover
local shadowing: either extend the XML input to include multiple <o> elements
with bases that will generate more than one match (so the caret-redundant
multi-match path is exercised) or add the new
doesNotReportWhenLocalShadowingRequiresHat test that constructs a nested object
with a locally shadowed name and asserts no defects; ensure assertions use
Matchers.hasSize(1) for the multi-match case or Matchers.emptyIterable() for the
negative case and keep line attributes present for accurate diagnostics.
|
@Snehakb1219-christ Hello! I've reviewed your branch named " |
|
@h1alexbel please, check this one |
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@Snehakb1219-christ please check
4 participants
Fixes #690
Summary by CodeRabbit
New Features
Tests