Conversation
Contributor
Author
|
Running clj-kondo on the current master produces the following set of issues: |
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 21 21
Lines 967 967
Branches 4 4
=======================================
Hits 952 952
Misses 11 11
Partials 4 4 Continue to review full report at Codecov.
|
Contributor
Author
|
@lvh, what are your thoughts on whether we should remove eastwood or leave it intact in parallel to clj-kondo? My only concern is they may potentially be issuing conflicting set of linting warnings for some of the arguable cases like formatting. |
Contributor
Author
|
I'll have a go at the issues reported by clj-kondo in a couple of days. |
Owner
|
I have no particular preference: I have never used them together, but I also have had good success with clj-kondo overall (though in this case the custom macro stuff we do does make it quite upset). Basically if someone will get the linter green I'm happy to take clj-kondo in and I'm willing to be swayed either way if eastwood should go (maybe it should). |
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
==========================================
+ Coverage 98.44% 98.47% +0.02%
==========================================
Files 21 21
Lines 967 982 +15
Branches 4 4
==========================================
+ Hits 952 967 +15
Misses 11 11
Partials 4 4
Continue to review full report at Codecov.
|
Contributor
Author
|
Alright, the linters are now happily green. I guess there's no harm in having both eastwood and clj-kondo for linting. Anyway, any of them can easily be removed later if one becomes a pain to support. On another note, I'd think about removing the |
Contributor
Author
|
I guess this is now ready for the review :) |
lvh
reviewed
Oct 15, 2020
| #_{:clj-kondo/ignore [:unused-import]} In | ||
| #_{:clj-kondo/ignore [:unused-import]} Out | ||
| #_{:clj-kondo/ignore [:unused-import]} Pinned | ||
| #_{:clj-kondo/ignore [:unused-import]} LongLong] |
Owner
There was a problem hiding this comment.
This should be a clj-kondo bug: these are used.
Contributor
Author
There was a problem hiding this comment.
Yeah, they are. I added the directives to make clj-kondo happy. I should probably revert that—that's a weird fix, you're right.
Skimming through the clj-kondo issues revealed clj-kondo/clj-kondo#911, which closely resembles the issue we have here. But the latest clj-kondo still gives the same unused import warnings.
I'll file an issue for our case in clj-kondo. Please stay tuned :)
Contributor
Author
There was a problem hiding this comment.
Well, I just gave it a second thought trying to come up with an example illustrating the issue to submit to clj-kondo, and I reckon the errors reported are actually fine, given the way things are implemented in caesium and the way clj-kondo handles macro expansions.
First of all, clj-kondo only expands a small set of macros as outlined here: https://cljdoc.org/d/clj-kondo/clj-kondo/2020.09.09/doc/configuration#unrecognized-macros
And then, all of the In, Out, Pinned, and LongLong usages happen to be in a quoted list in caesium. In such form they are not bound to the imported symbols until they are later expanded in the defsodium macro.
So, clj-kondo, albeit being pretty naïve here, correctly complains the imports are not used.
I'm not sure what to do with this. Any ideas?
Owner
There was a problem hiding this comment.
Hm, OK, I think it's still a bug but I see your point that it's unreasonable to expect clj-kondo to fix this especially on short timeframes. I think this is good to go as is then?
Owner
|
From borkdude in the slack: I think the cleanest way is to make defconsts take varargs and tell clj-kondo to lint it as |
Contributor
Author
|
I’ll have a closer look at this in the coming days. |
Contributor
Author
|
Whoa, time flies. It's been five years since I've dropped the ball on this one. I'm a bit occupied with other things nowadays and seemingly won't come back to this PR any time soon. :-( |
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
4 participants
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.
Where it all started
The preference isn't a strong ground to change the tool, albeit I tend to use clj-kondo in my other projects as well. :) The warnings were quite easy to fix, so just sticking with it is fine I guess for now. I doubt clj-kondo will be any different in terms of integrating in GH Actions later. Please tell me if you'd like eastwood replaced and we can look into it at a later stage.
Originally posted by @eploko in #69 (comment)
What this PR brings
This adds clj-kondo as a dependency for the test profile. It also adds a step to the build workflow to lint the code with clj-kondo. The step is postfixed with
|| truea là the eastwood step so it doesn't fail the build.Things to do before merging this in