Skip to content

Fix some linter errors#338

Merged
niyarin merged 2 commits intomasterfrom
fix/linter-error
Mar 25, 2025
Merged

Fix some linter errors#338
niyarin merged 2 commits intomasterfrom
fix/linter-error

Conversation

@alumi
Copy link
Copy Markdown
Member

@alumi alumi commented Mar 25, 2025

This PR fixes coding styles of test codes and does not introduce any changes of its behavior.

clj-kondo reports test/cljam/io/gff_test.clj:319:70: error: Unresolved symbol: ?str on the master branch.
I've removed the type hints in 909e6d2 because adding type hints to the symbols in expr of clojure.test/are has no effects on suppressing reflections.
In f13581a, I've also fixed some coding styles warned by kibit.

Details

The strange behavior of the :unresolved-symbol error is due to differences in how metadata is handled.
clojure.test/are uses clojure.template/do-template which replaces symbols using clojure.walk/postwalk-replace.
Since a symbol’s metadata does not affect its identity during replacement, this process works as expected.
However, clj-kondo emulates clojure.template/do-template with clojure.walk/postwalk-replace, but it operates on AST nodes, which have :meta fields directly attached.
This difference causes replacement failure.
As seen above, since clojure.test/are replaces symbols in its expr, we need type information on args rather than on the symbols in expr.

user> (set! *warn-on-reflection* true)
user> (def s "A")
user> (are [x] (.length ^String x) s)
Reflection warning, *cider-repl chrovis/cljam:localhost:64423(clj)*:79:16 - reference to field length can't be resolved.
1
user> (are [x] (.length x) ^String s)
1

In the case of this PR, adding type hints is unnecessary because args are already typed vars.

@alumi alumi requested a review from a team as a code owner March 25, 2025 04:20
@alumi alumi requested review from niyarin and r6eve and removed request for a team and r6eve March 25, 2025 04:20
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.00%. Comparing base (9408e13) to head (f13581a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   89.96%   90.00%   +0.04%     
==========================================
  Files         104      104              
  Lines        9723     9723              
  Branches      529      529              
==========================================
+ Hits         8747     8751       +4     
+ Misses        447      445       -2     
+ Partials      529      527       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@niyarin niyarin left a comment

Choose a reason for hiding this comment

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

Thank you for the fix.
I confirmed that the issue with the linter warning in test/cljam/io/gff_test.clj has been resolved.

@niyarin niyarin merged commit 2c12beb into master Mar 25, 2025
17 checks passed
@niyarin niyarin deleted the fix/linter-error branch March 25, 2025 23:04
@alumi
Copy link
Copy Markdown
Member Author

alumi commented Mar 26, 2025

@niyarin Thank you for reviewing! 🙏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants