Feature(#797): add +syntax in meta#798
Feature(#797): add +syntax in meta#798Daniilmipt wants to merge 10 commits intoobjectionary:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new lint that validates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
🤖 Fix all issues with AI agents
In `@src/main/java/org/eolang/lints/LtSyntaxVersion.java`:
- Around line 115-117: The validation method valid() accepts semver strings with
pre-release suffixes (e.g. "0.59.0-beta") while parts() strips the suffix before
numeric comparison, making "0.59.0-beta" compare equal to "0.59.0"; either
forbid pre-release suffixes in valid() or implement proper pre-release handling:
update valid() to not accept "-..." suffixes (remove the optional
"-[a-zA-Z0-9-]+" from the regex) if you want to treat only plain semver, or
modify parts() (and the comparison logic that uses parts()) to preserve and
return the pre-release token and add a pre-release comparator that enforces
SemVer precedence (pre-release < no pre-release, and lexicographic/numeric
ordering of dot-separated identifiers) so comparisons using parts()/the
comparator respect SemVer rules.
- Around line 47-49: The constructor LtSyntaxVersion(final String parserVersion)
must validate parserVersion (non-null, non-empty, and valid SemVer) to avoid
NumberFormatException later in parts(); add validation in the LtSyntaxVersion
constructor that checks parserVersion against a SemVer pattern (or attempts to
parse via the existing parts() helper) and throws an IllegalArgumentException
with a clear message (including the invalid value) if it fails; reference the
LtSyntaxVersion constructor, the parserVersion field and the parts() method when
implementing this defensive check.
In `@src/main/java/org/eolang/lints/MonoLints` copy.java:
- Around line 1-66: Remove the accidental duplicate source (the class named
MonoLints copy) and instead update the original MonoLints class to include the
new LtSyntaxVersion in its LINTS list; specifically, ensure MonoLints.LINTS
contains new LtAsciiOnly(), new LtReservedName(), new LtSyntaxVersion() (in that
list), and leave the rest of the constructor logic (LtIncorrectUnlint,
LtUnlintNonExistingDefect, WpaLints, WpaLintNames) unchanged so behavior and
mutual-ignore names are preserved.
🧹 Nitpick comments (1)
src/test/java/org/eolang/lints/LtSyntaxVersionTest.java (1)
107-131: Fragile:.iterator().next()can throwNoSuchElementExceptionifdefectsis empty.Both
reportsErrorSeverityForInvalidFormatandreportsErrorSeveritydirectly call.iterator().next()on the defects collection. If the lint unexpectedly returns zero defects due to a bug, the test fails with a confusingNoSuchElementExceptionrather than a meaningful assertion message.Consider asserting the collection is non-empty first, or using Hamcrest's
hasItemwith a severity matcher to get a clear assertion failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/org/eolang/lints/LtSyntaxVersionTest.java`:
- Around line 160-174: The test method rejectsInvalidParserVersion calls
Assertions.assertThrows three times without explanation messages causing qulice
to fail; update the three assertThrows in
LtSyntaxVersionTest#rejectsInvalidParserVersion so each call to
Assertions.assertThrows provides a descriptive message (e.g., "should reject
'latest' version", "should reject empty version", "should reject null version"),
or alternatively annotate the test with
`@SuppressWarnings`("JTCOP.RuleAssertionMessage") if you prefer to silence the
rule; reference the test method name rejectsInvalidParserVersion and the
LtSyntaxVersion constructor when making the change.
🧹 Nitpick comments (2)
src/main/java/org/eolang/lints/LtSyntaxVersion.java (1)
153-160:Splitterimport for a single trivial split — considerString.splitinstead.
Splitter.on('.').splitToList(...)pulls in Guava'sSplitterfor whatversion.split("-", 2)[0].split("\\.")achieves natively. Minor, but it's an extra dependency for no added safety here (the input is already validated).src/test/java/org/eolang/lints/LtSyntaxVersionTest.java (1)
60-80: Two independent assertions in one test method.
usesDefaultVersionWhenNoArgCtorasserts two distinct behaviors (no defect for0.0.0, one defect for0.0.1). If the first assertion fails, the second is never reached. Consider splitting into two@Testmethods for independent failure reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/eolang/lints/LtSyntaxVersion.java`:
- Around line 149-164: The method newer() uses negative checks (major != 0 /
minor != 0) which triggers PMD's ConfusingTernary; update newer() to use
positive comparisons or early returns: compute left and right via
LtSyntaxVersion.parts(this.parser) and LtSyntaxVersion.parts(syntax), then
compare major, minor, patch using Integer.compare but branch with == 0 checks
(or return immediately when a comparison is non-zero) so the logic reads
positively (e.g., if (major == 0) { ... } else return major < 0), ensuring the
same boolean result while satisfying PMD.
🧹 Nitpick comments (1)
src/test/java/org/eolang/lints/LtSyntaxVersionTest.java (1)
66-86: Consider splitting two independent assertions into separate@Testmethods.
usesDefaultVersionWhenNoArgCtortests two distinct behaviors (no defect for0.0.0and one defect for0.0.1). If the first assertion fails, the second is never executed. Splitting into two tests improves failure diagnostics.
|
@Daniilmipt the implementation looks excellent, thanks! However, we don't use this new class in the |
Thanks! You are right. I added this. Can you tell me if there is instruction somewhere on how to run the linter locally using the example of I found how to run from the |
| new LtAsciiOnly(), | ||
| new LtReservedName() | ||
| new LtReservedName(), | ||
| new LtSyntaxVersion() |
There was a problem hiding this comment.
@Daniilmipt this will set the version to 0.0.0, which is not what we need, right? Maybe, you can get current version of from Manifests.read("EO-Version").
|
@Daniilmipt read this: https://www.yegor256.com/2025/05/25/bug-driven-development.html Don't ask questions like this. Instead, report a new bug, complaining about the quality of our documentation. |
2 participants
We should compare parser version with version from meta
+syntaxand be sure to detect this meta "+syntax"Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Behavior
Tests