Parinfer based style checker?


#1

I wonder if there would be an easy way to make a tool that uses parinfer to “style check” clojure code. The use case I have in mind is that I have a fear of using parinfer for work projects because I don’t want to reformat code I’m not changing… but if I could tell everyone else hey this doesn’t conform to proper styling, it is easier to convince people that the correct thing is to reformat the code. This is pretty hypothetical, but I think the kernel of an idea here is that a style checking utility sister to parinfer might be handy? Just something to think about.


#2

I think this is what you’re after: https://github.com/shaunlebron/parlinter

I made that tool last year for the reasons you mentioned. Formatting checkers don’t seem at all accepted in the Clojure community yet, but there was an interesting discussion on how we have all the right tools today if it was desired: https://twitter.com/shaunlebron/status/952861330276483072


#3

Awesome! As usual you are 2 steps ahead already :slight_smile:
I just ran it on our repo and as expected there are many updates… but I think it’s reasonable on our small team for me to do a whitespace only commit and tell everyone to pull it before making another change.

As for adoption, I can’t speak for the community as a whole but I can speak for myself; I don’t see any valid objection to it, I think in my case there is just a lack of awareness about how to do it… phrased another way: For work projects I’d like our CI server to skwark when people added egregiously bad formatting. While I like the current parlinter very much, I’m not sure how I’ll integrate it with the built chain. What I think I’d prefer to do is write a single test that checked all the source code formatting. That way I don’t have to include parlinting in my build chain at all, it just becomes a test which the CI server can fail, and people will test locally before they check in. Unfortunately my tests are in Clojure and parlinter is in javascript so I can’t really do that. I suspect if I use parinfer lib in Clojure I can write the test I want and be happy.

I want to stress that these aren’t complaints, I’m just relating some of what I perceive to be the barriers to adoption.

I’m certainly not someone to advocate strict conventions over source code; I value flexibility and people having their own tools/styles… BUT I do think that parlinter provides an excellent common ground; if your code doesn’t fit, it’s really outside of anything reasonable. I’d love to adopt it in my projects and work.

One behavior I observed that confused me was:

  •    ["N" "N"] { "manual" 0.00005 "shows" 2317457 "clicks" 283 "conversions" 37 }}
    
  •    ["N" "N"] { "manual" 0.00005 "shows" 2317457 "clicks" 283 "conversions" 37}}
    

parlinter removed the SPACE before the closing.
That seemed weird to me… I couldn’t see this as necessary for parinfer?

Just to beat the drum a bit more on why I think the test approach in particular might go further: I’ve tried plugins in the past and honestly I just forget to run them. And if they are on the build server they just cause back and forth, and nobody likes broken builds, so they blame the linter. Tests on the other hand are something I love to run locally before I commit something.

What do you think?


#4

There is another alternative, which will be in the new parinfer integration in Cursive. I’m hopeful this will be out Real Soon Now, I have a couple of outstanding bugs which I thought were in parinfer but after investigation seem like they’re bugs in my port of it. With any luck they’ll be quick to find and squash.

Instead of automatically adjusting the indentation using paren mode, it uses paren mode to mark where the indentation is incorrect, and disables parinfer until the user has fixed that by hand. It looks like this:

I’m also planning to make this per top-level form: parinfer will only be disabled for a particular top-level form if the indentation in that form is incorrect - currently it’s per file. If I can make it work as I imagine it, this means that you’ll only have to correct the indentation in the parts of the file you actually touch, and the rest can be incorrectly indented with no problem. This currently only works in my head though :slight_smile:.


#5

Hi Colin :slight_smile:
Cool! That will be very helpful.


#6

Thanks for your thoughts! I think there are a few blockers for adoption as well. I’ll address ones you brought up, and add others.

No clear integration. I would like to make it easier to integrate Parlinter into Clojure workflows, by e.g. running as a test. This could be built using parinfer-jvm. Some also want to add it as precommit-hook.

Close-paren spacing. You’re the second person to bring this up! This was reported here first. We currently just blow away the spaces before close-parens at the end of a line, since I want that region to be deterministically computed from open-parens and indentation only. But I see no reason that we can’t add a rule to mirror the space of the open-paren (yet):

;; input
( foo

;; actual
( foo)

;; expected
( foo )

Indented comment UX. I also want to say that this issue is probably a big blocker for parlinter adoption, since it currently “breaks UX for non-parinfer users” by pushing comments outside of expressions. There’s some details in the issue about why this is pretty bad for people who don’t use parinfer.

;; before
(foo
  bar
  ;; comment
  )

;; after
(foo
  bar)
  ;; comment

All this said, my first priority is to make Parinfer’s new Smart Mode work well, and the second priority is to address this issue of lowering the friction of Parinfer’s formatting conflicts on projects. I appreciate you bringing this up—it’s something that should be fixed.


#7

Hi Shaun,

Thanks for the pointers.

I got a proof of concept going here:

If you run the tests for this project, you’ll see a failing test!
The culprit is an egregious use of trailing parenthesis degeneracies. If you fix the trailing paren, the test passes again.

The test is checking that all files in “src” and “test” conform to parinfer mode. The test does not change any files, it leaves that to your discretion.

My feeling is that it’s preferable over hooks
because hooks aren’t part of my workflow… whereas I do run tests
during development and before I commit/push.
I’m interested in how other people react to this.
I’d love to be able to say “problem 1 solved, now there is a clear integration path”,
but I suspect some people wont like the test approach.

So on the one hand it is actually really easy to set up,
on the other hand I have some frustrations to vent!
Please take my whining below with a grain of salt…
I’m very thankful for Parinfer and especially that the code/libraries are available
to use and examine. :slight_smile:

/begin whining
Parinfer versioning irks me. There are many parinfer artifacts of various age and progeny which contribute to uncertainty about what I’m getting. My “Parsnip” example depends on cross-parinfer from @zoakes. Why aren’t I using parinfer-jvm you ask? Well I cloned parinfer-jvm and followed the readme… did a gradlew install and something happened but there was no indication where any artifact went. So I looked in my ~/.m2 and saw ~/.m2/repository/parinfer/parinfer/0.2.3/ from about 6 months ago… that’s not it. So I gave up and went searching and found cross-parinfer. I looked in his deps.edn and didn’t see any parinfer-jvm… I did see a parinfer 0.4.0 but had no idea if that was parinfer-jvm, and if it was if it was the latest version. But at least I was able to depend on cross-parinfer from clojars so I tried it and was able to navigate through the source to see how it was using parinfer. Now I go back and look in parinfer-jvm and notice a build directory with a lib/parinfer-0.4.0.jar … so I know that’s where it went and that cross-parinfer has the latest version. BUT I look at the github for parinfer-jvm and the last commit was 2 years ago. Given recently described changes in parinfer, I have low confidence that this will behave in the way that parlinter does. And I have uncertainty whether @colinfleming is using this implementation, or writing his own.
The impediments were easily overcome with some persistence. I don’t want to distract you from Smart Mode. I just want to make a honest report of my experience.
/end whining

I’ll improve the test output to be friendlier and try it out on a larger repository some time soon.

TLDR: It’s actually trivially easy to do parlinting as a test.


#8

Just a thought on how to improve the ability to talk about versions…

It would be possible to have all “satellite” parinfer projects be versioned according to the json test case files. ie: all projects claiming to offer parinfer could reference that version as their implementation level. To me that provides the connection between this version number gives you this behavior.

For example parinfer-jvm currently has the test cases copied across from some particular point in time which I think equates to parinferjs 1.7.0

As such I think it would be more helpful to advertise parinfer-jvm as being “parinfer 1.7.0” instead of the current situation "parinfer 0.4.0"
This would be most helpful as the artifact version itself, but it could also just be information available in the README or project.clj.
Furthermore parinfer-jvm should probably DEPEND on a version of parinferjs just for the test cases. Then it would be a firm guarantee that parinfer-jvm meets the tests for parinferjs x.y.z
I’ll look into whether there is a way to do this or not.


#9

I like the idea of versioning the satellite libraries according to the version of the tests that they conform to.

What I use in Cursive was based on parinfer-jvm a long time ago. I do plan to fix it up once what I have is working stably, but there didn’t seem to be any urgency since as far as I know no-one uses it. Zach is the only client I’m aware of for parinfer-cross, and he said that he never uses the JVM part of that and doesn’t know of anyone else using it.

Unfortunately it’s not a trivial thing to merge the changes back. My implementation has diverged fairly significantly from the original, I also removed numerous features that I don’t use and added some which are not in the original (some of which may be there at some point, like returning edits rather than the whole doc). It’s not something that will happen right away, sadly.


#10

sorry about the confusion over versioning, I do realize it’s hard to track those! Chris was initially against tying the port versions together when I first brought it up, but he may be open to it a second time. He currently manages most of the ports, but I agree this seems like a good idea.

I suppose in the meantime, there can at least be a table in the main parinfer readme about compatibility.

thanks for putting together the parsnip proof of concept! this code is definitely a very short and simple test to write :slight_smile: