Fix all linter warnings and errors in test suite

[DerGuteMoritz]

Decided to start small since I already encountered a few issues that I wanted to coordinate on before continuing. I will add comments on the diff to discuss these!

[KingMob]

@DerGuteMoritz What's the status of this? I was looking forward to it

[DerGuteMoritz]

@KingMob I'll get back to it soon - work got a bit busy after I opened this but should be back in more quiet waters in a few days!

[KingMob]

No worries, take your time.

On Mon, Apr 11, 2022 at 6:56 PM Moritz Heidkamp @.***> wrote:

@KingMob https://github.com/KingMob I'll get back to it soon - work got a bit busy after I opened this but should be back in more quiet waters in a few days!

— Reply to this email directly, view it on GitHub https://github.com/clj-commons/aleph/pull/583#issuecomment-1094905607, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHHB5MCNILMLV4NF4WUQO3VEQAPLANCNFSM5REK6BCQ . You are receiving this because you were mentioned.Message ID: @.***>

[KingMob]

@DerGuteMoritz Any updates?

[DerGuteMoritz]

@KingMob Sorry for the delay :bow: Back on track now. I've rebased and adjusted for the latest master changes and incorporated your feedback from above (see threads there for some further details). I want to look into adding a circle-ci job for clj-kondo and then this could be merged!

[DerGuteMoritz]

Alright, just added a lint job to the Circle CI config. Don't know how to test it, though - it doesn't seem to be picked up automatically for PR checks, at least. Hope you know what to do :pray:

[KingMob]

I need to check with borkdude on using kondo for circleci; I think Eastwood is preferred for non-interactive CI linting because it more closely mimics how the real compiler sees code. Let me get back to you on that.

[KingMob]

Alright, just added a lint job to the Circle CI config. Don't know how to test it, though - it doesn't seem to be picked up automatically for PR checks, at least. Hope you know what to do 🙏

@DerGuteMoritz I think the reason it's not picked up is the indentation level makes it a separate job, but we probably want to place it under the existing build job as another step (preferably right before the testing step). In general, clj-commons has used just two circleci jobs, one for building and one for deploying (deployment jobs don't yet exist for aleph and manifold for historical reasons, since their deployment isn't yet under the clj-commons maven group).

[KingMob]

I see plenty of people using kondo for CI, so that's fine. It might be wiser to make a separate ci config file, as described in https://github.com/clj-kondo/clj-kondo/blob/master/doc/ci-integration.md

The goal should be to get it in place now, with nothing blocking merges. Aleph and Manifold do some funky stuff sometimes, and kondo should not block current development just because of that. As we modernize the codebase, we can revisit what warnings we want to turn back on.

[DerGuteMoritz]

A follow-up for this PR would be to format using cljfmt. Using any parinfer/paredit is really complicated on the state of the codebase.

Indeed, the formatting is quite idiosyncratic in places, mainly due to all these macros I think. Running cljfmt on it sounds like a nice follow-up indeed. I'll see if I can tackle that, too, once we've got the linting in :smile_cat:

@DerGuteMoritz I think the reason it's not picked up is the indentation level makes it a separate job, but we probably want to place it under the existing build job as another step (preferably right before the testing step).

I broke it out into a separate job for two reasons:

1. This way, the lint job will show up as a separate check on a PR, giving immediate indication of what kind of failure there is.
2. A different Docker image is required for running clj-kondo. AFAIK there is no way to combine two existing images into one so to achieve that one would have to either build a custom image or install clj-kondo from within the job which is pretty messy.

In general, clj-commons has used just two circleci jobs, one for building and one for deploying (deployment jobs don't yet exist for aleph and manifold for historical reasons, since their deployment isn't yet under the clj-commons maven group).

And is there a technical rationale for doing this or just tradition / convention? :smile:

I see plenty of people using kondo for CI, so that's fine.

Yup, the way it operates is pretty much intended for non-interactive use. Also, without an automated job like this, it's pretty much guaranteed that the code will evolve into a non-linted state pretty quickly unless reviewers diligently run the linter before merging. And since that's so trivially automated ... :smile:

It might be wiser to make a separate ci config file, as described in https://github.com/clj-kondo/clj-kondo/blob/master/doc/ci-integration.md

This would only be needed if we want CI-specific config which currently doesn't seem necessary nor particularly desirable - why would you want to have different linting rules for development vs. CI? :thinking:

That being said, I'm happy to adjust any of the above to your liking, just gimme the word :+1:

[DerGuteMoritz]

Sorry, missed one bit:

The goal should be to get it in place now, with nothing blocking merges. Aleph and Manifold do some funky stuff sometimes, and kondo should not block current development just because of that. As we modernize the codebase, we can revisit what warnings we want to turn back on.

If by that you mean that lint failures should not block PRs from getting merged, that would be another case in point for having it be a separate CI job so that it can be marked as non-blocking.

[KingMob]

We can try adding automatic cljfmt, see how it goes. In my limited experience with it, it requires a bunch of config for good results, especially around macros.

I'm fine with treating linting as a separate job, as long as we can ensure dependencies. E.g., if either linting or building/testing fail, a deploy job should not be runnable (see other clj-commons repos for examples). Regardless, people should always be able to click into CircleCI for details, so making it directly visible in GH isn't a big priority to me.

And is there a technical rationale for doing this or just tradition / convention?

I don't know if there's a good reason the build job can't be split up, but I do know the other clj-commons deployment jobs run on different git tags, and the branch/tag criteria is set at the job level. So...maybe?

A different Docker image is required for running clj-kondo.

I didn't notice you used a different docker image. Potentially dumb question: does this work correctly in circleci? I know in other clj-commons Circle setups, we download a Babashka script for deployment. We could do something similar to download kondo if necessary. (We should also upgrade to the newer circleci clojure img...)

If by that you mean that lint failures should not block PRs from getting merged, that would be another case in point for having it be a separate CI job so that it can be marked as non-blocking.

But some rules SHOULD be hard failures (e.g., arity errors, unresolved symbols), just not necessarily all. Here's an example of why we might want separate CI and dev configs: https://github.com/clj-commons/manifold/blob/dbdd600c5a48df314e3ad06fe2986e05e5af21c1/src/manifold/time.clj#L126-L128. Zach does some crazy ns stuff here. At the developer level, I want to be reminded to fix this, but I don't want to have this block CI, because it may take some time to fix.

Validating correctness is more important, but as you point out, maintaining good style requires regular linting, too. We could make all the changes at once and then enable them as blockers in the same PR, but I'm afraid this is turning out to be a lot of work already. I prefer smaller, more focused PRs; they're easier to finish, easier to review, and less risky to merge.

If you don't want to have separate dev/CI configs (which is fair), let's still try to split up this work into the following PRs:

1. A kondo and kondo-based fix PR, no circleCI
2. A cljfmt merge that does nothing but update the indentation based on the Clojure style guide and specify the indentation rules
3. A circleCI PR that adds cljfmt and kondo jobs or steps

[DerGuteMoritz]

We can try adding automatic cljfmt, see how it goes. In my limited experience with it, it requires a bunch of config for good results, especially around macros.

Yeah, this will likely be some work - same with clj-kondo actually :grimacing:

I'm fine with treating linting as a separate job, as long as we can ensure dependencies. E.g., if either linting or building/testing fail, a deploy job should not be runnable (see other clj-commons repos for examples).

Ah, right, looks like we would have to set up a workflow for this, similar to what's done e.g. here: https://github.com/clj-commons/byte-streams/blob/master/.circleci/config.yml#L7-L22 - looks like it should be possible to just add the lint job as another requires dependency to the deploy job.

I didn't notice you used a different docker image. Potentially dumb question: does this work correctly in circleci?

Fairly sure! Docs say this about image: "The name of a custom docker image to use. The first image listed under a job defines the job’s own primary container image where all steps will run."

I know in other clj-commons Circle setups, we download a Babashka script for deployment. We could do something similar to download kondo if necessary.

Right, that's the approach I referred to as "messy" above :smile:

But some rules SHOULD be hard failures (e.g., arity errors, unresolved symbols), just not necessarily all. Here's an example of why we might want separate CI and dev configs: https://github.com/clj-commons/manifold/blob/dbdd600c5a48df314e3ad06fe2986e05e5af21c1/src/manifold/time.clj#L126-L128. Zach does some crazy ns stuff here. At the developer level, I want to be reminded to fix this, but I don't want to have this block CI, because it may take some time to fix.

OK makes sense! But I think I would rather have the default developer config be the same as the one used by CI so that one doesn't get all kinds of warnings during development which end up being irrelevant. So far I didn't have to disable any linting rules globally but once we do, we could provide a separate config which one could use instead for "full" linting when desired.

We could make all the changes at once and then enable them as blockers in the same PR, but I'm afraid this is turning out to be a lot of work already. I prefer smaller, more focused PRs; they're easier to finish, easier to review, and less risky to merge.

Aye, agree with you here. That's why I decided to make this first PR about the test suite only so that we can settle these discussions before having invested a ton of work on the main code base :sweat_smile:

If you don't want to have separate dev/CI configs (which is fair), let's still try to split up this work into the following PRs:

1. A kondo and kondo-based fix PR, no circleCI
2. A cljfmt merge that does nothing but update the indentation based on the Clojure style guide and specify the indentation rules
3. A circleCI PR that adds cljfmt and kondo jobs or steps

Sounds like a plan :+1: I will drop the CI job from this PR then. Shall we go ahead with merging it as-is, i.e. test suite only or would you prefer a big one for the whole code base now that we've mostly settled the nitty-gritty? FWIW I think I would prefer to merge this one as-is because it already provides some value and it will probably take a while for me to fix up the main code.

[DerGuteMoritz]

@KingMob There you go!