Skip to content

fix(#4949): enabled object-has-data lint#4972

Open
eshabakhov wants to merge 2 commits intoobjectionary:masterfrom
eshabakhov:#4949
Open

fix(#4949): enabled object-has-data lint#4972
eshabakhov wants to merge 2 commits intoobjectionary:masterfrom
eshabakhov:#4949

Conversation

@eshabakhov
Copy link
Copy Markdown
Contributor

@eshabakhov eshabakhov commented Apr 2, 2026

Summary

This PR solves #4949

Changes

  • enabled object-has-data lint

Summary by CodeRabbit

  • Chores

    • Removed an unused build-time lint suppression from the build configuration, reducing suppressed lint rules.
  • Refactor

    • Simplified how package-name formatting is produced internally, producing shorter, more consistent forma strings.
  • Tests

    • Updated tests to expect the new, simplified package naming in generated forma outputs.

@github-actions github-actions Bot added the core label Apr 2, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 781b8507-4b65-404d-a94e-3ba55d9648dc

📥 Commits

Reviewing files that changed from the base of the PR and between 6346f3c and 7dc9800.

📒 Files selected for processing (1)
  • eo-runtime/src/main/java/org/eolang/PhDefault.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • eo-runtime/src/main/java/org/eolang/PhDefault.java

📝 Walkthrough

Walkthrough

Removed an object-has-data lint suppression from Maven config and simplified PhDefault.forma() to use PhPackage.GLOBAL for name construction; adjusted a test expectation accordingly.

Changes

Cohort / File(s) Summary
Maven Configuration
eo-runtime/pom.xml
Removed the <lint>object-has-data</lint> entry and the @todo #4538`` comment from the eo-maven-plugin `compile` execution's `` list.
Core Implementation & Tests
eo-runtime/src/main/java/org/eolang/PhDefault.java, eo-runtime/src/test/java/org/eolang/PhDefaultTest.java
Deleted the TO_FORMA regex and related package-derived logic; forma() now builds names using PhPackage.GLOBAL + . + name. Updated test expectation from Φ.org.eolang.number to Φ.number.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • maxonfjvipon
  • yegor256

Poem

🐰
I hopped through code with nimble cheer,
Took out a regex, kept it clear,
PhPackage leads — a simpler song,
Lint suppression gone along,
Bright carrots for tests that cheer!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title claims to 'enable' object-has-data lint, but the changes show it was removed from suppressed lints list, which is contradictory to the stated objective. The title should accurately reflect that the lint suppression was removed to enable the lint, or clarify the actual change. Consider revising to 'fix(#4949): remove object-has-data lint suppression' or similar.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 2, 2026

🚀 Performance Analysis

All benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information.

Click to see the detailed report
Test Base Score PR Score Change % Change Unit Mode
benchmarks.XmirBench.xmirToEO 223.913 193.004 -30.909 -13.80% ms/op Average Time

✅ Performance gain: benchmarks.XmirBench.xmirToEO is faster by 30.909 ms/op (13.80%)

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
eo-runtime/src/test/java/org/eolang/PhDefaultTest.java (1)

404-410: Test description is now misleading.

The assertion message at line 406 says "forma of 'number' is the full name of the 'number' object", but "Φ.number" is no longer the "full name" (which would include the package path like org.eolang). Consider updating the description to accurately reflect the new format.

📝 Suggested description update
     `@Test`
     void rendersFormaProperly() {
         MatcherAssert.assertThat(
-            "forma of 'number' is the full name of the 'number' object",
+            "forma of 'number' is the global prefix with the object name",
             new Data.ToPhi(42L).forma(),
             Matchers.equalTo("Φ.number")
         );
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@eo-runtime/src/test/java/org/eolang/PhDefaultTest.java` around lines 404 -
410, Update the test description in PhDefaultTest.rendersFormaProperly to
accurately reflect the current forma format (short name "Φ.number" rather than a
fully-qualified name); change the assertion message string passed to
MatcherAssert.assertThat to something like "forma of 'number' is the short/name
representation" or "forma of 'number' is 'Φ.number'" so it no longer claims it
is the full name produced by Data.ToPhi.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@eo-runtime/src/test/java/org/eolang/PhDefaultTest.java`:
- Around line 404-410: Update the test description in
PhDefaultTest.rendersFormaProperly to accurately reflect the current forma
format (short name "Φ.number" rather than a fully-qualified name); change the
assertion message string passed to MatcherAssert.assertThat to something like
"forma of 'number' is the short/name representation" or "forma of 'number' is
'Φ.number'" so it no longer claims it is the full name produced by Data.ToPhi.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a062486a-3dbf-4c83-9f5b-3480180a616f

📥 Commits

Reviewing files that changed from the base of the PR and between 4cb0d56 and 6346f3c.

📒 Files selected for processing (3)
  • eo-runtime/pom.xml
  • eo-runtime/src/main/java/org/eolang/PhDefault.java
  • eo-runtime/src/test/java/org/eolang/PhDefaultTest.java
💤 Files with no reviewable changes (1)
  • eo-runtime/pom.xml

@eshabakhov
Copy link
Copy Markdown
Contributor Author

eshabakhov commented Apr 3, 2026

@yegor256 idk it's my fault or not.

Qulice does not produce any errors when running locally.

I checked object-has-data linter.
If I'm wrong, it should prints this message https://github.com/objectionary/lints/blob/master/src/main/resources/org/eolang/lints/critical/object-has-data.xsl#L27

Not this https://github.com/objectionary/eo/actions/runs/23890624621/job/69662894113?pr=4972#step:5:9197

@yegor256
Copy link
Copy Markdown
Member

yegor256 commented Apr 3, 2026

@eshabakhov check this out: https://www.yegor256.com/2025/11/09/help-me-cant-merge.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants