Skip to content
This repository has been archived by the owner on Jan 28, 2019. It is now read-only.

Add cljfmt as a linter #72

Merged
merged 4 commits into from
Mar 16, 2016
Merged

Add cljfmt as a linter #72

merged 4 commits into from
Mar 16, 2016

Conversation

arrdem
Copy link
Collaborator

@arrdem arrdem commented Feb 24, 2016

This changeset adds cljfmt as a linter, checking both the Clojure sources and the Clojure tests. It's not perfect, but it's better than nothing.

Also reformats both sources sets so that the linter runs cleanly.

Fixes #31

@arrdem arrdem force-pushed the linter/cljfmt branch 3 times, most recently from 37bbaae to c88282c Compare February 24, 2016 23:35
@arrdem
Copy link
Collaborator Author

arrdem commented Feb 24, 2016

Performance concerns with cljfmt completely mitigated as of weavejester/cljfmt#58. Formatting limitations (or at least differences with the many varied styles within core) remain so this is by no means a complete fixup despite high LoC impact.

Among unsupported features are ns autoformatting, single semicolon to double semicolon conversion depending on comment placement and any sort of standardize def/defn styling.

@danielcompton
Copy link
Contributor

Will take a look at this, I think the formatting could be a little more optimal. Probably best to do it all in one fell swoop.

@arrdem
Copy link
Collaborator Author

arrdem commented Feb 25, 2016

[cljfmt "0.4.1"] and its lein plugin companion have been released with my performance improvements, so they should be swapped in instead of my cljfmt distributions.

@arrdem arrdem force-pushed the linter/cljfmt branch 3 times, most recently from 2b35c0a to 4e7b729 Compare March 3, 2016 00:45
@arrdem
Copy link
Collaborator Author

arrdem commented Mar 3, 2016

Remaining concerns:

  • defn formatting is all over the place in core, cljfmt doesn't help with this. In the single arity case is a defn parenthesized. Is ^{} metadata preferred on the naming symbol or should the defn supported metadata map be used. Should ^{:doc ..} be used or should the optional docstring be used. On and on and on.
  • def metadata is all over the place in core, cljfmt doesn't help with this. Many of the same issues as with defn.
  • cljfmt doesn't paragraph flow docstrings to the project standard 100chr width.
  • cljfmt doesn't vertically align bindings (if that's even a goal).
  • cljfmt doesn't vertically align maps (if that's even a goal).
  • core is all over the place in terms of using (or not using) import. Maybe that's a separate ticket, maybe it isn't.
  • There are instances of Java style local naming for no apparent reason.

@arrdem
Copy link
Collaborator Author

arrdem commented Mar 3, 2016

Honestly dinking with this more is making me more inclined to just merge this since it provides a low bar for lisp formatting, and we can raise that bar incrementally rather than trying to do a monolithic reformat. In part because it feels like doing this 'right' involves writing an official style guide up front, which I don't feel is a great use of time.

@arrdem arrdem self-assigned this Mar 3, 2016
@arrdem arrdem force-pushed the linter/cljfmt branch 9 times, most recently from e7ee2ed to 5b66e7b Compare March 13, 2016 04:19
@arrdem
Copy link
Collaborator Author

arrdem commented Mar 14, 2016

Yeah after more dinking I'm really not convinced that this is worth the up front effort of imposing more code style than cljfmt can automate. Unless there is opposition to the point, I'll just fix the build and merge this tomorrow.

Would it be nice if one interop style were preferred yes. Would it be nice if metadata notation were consistent yes. Is it worth my time to rewrite all of core by hand one fn at a time... probably not since what's there works.

@arrdem arrdem force-pushed the linter/cljfmt branch 2 times, most recently from f6cde1a to c44fc68 Compare March 15, 2016 17:40
arrdem added a commit that referenced this pull request Mar 16, 2016
@arrdem arrdem merged commit a882086 into develop Mar 16, 2016
@arrdem arrdem deleted the linter/cljfmt branch March 16, 2016 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants