Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the enforce-ns-style transform #85

Merged
merged 1 commit into from Mar 15, 2021
Merged

Add the enforce-ns-style transform #85

merged 1 commit into from Mar 15, 2021

Conversation

cespare
Copy link
Owner

@cespare cespare commented Feb 10, 2021

This applies common NS style rules from "How to ns".

See the updated README for an exact list of the changes that it does. (I just went with the ones that were really easy.)

@jwhitbeck please review the changes that this implements (no need to review the code itself). You can see this applied to our internal codebase on the branch caleb.cljfmt-how-to-ns. (Right now we're just agreeing about the behavior we want; actually rolling out this change internally will be a job for later.)

Fixes #68.

@cespare cespare requested a review from apesic February 10, 2021 23:44
@jwhitbeck
Copy link
Collaborator

jwhitbeck commented Feb 11, 2021

Thanks for tackling this. I looked through the changes, and I really like the new standardized output.

I noticed two recommendations from the How to ns artice, that don't yet appear to be implemented:

  • sorting, in particular within the :refer vector, the imported pacakges, and the class names inside an imported prefix.
  • :as before :refer

@cespare
Copy link
Owner Author

cespare commented Feb 11, 2021

I noticed two recommendations from the How to ns artice, that don't yet appear to be implemented:

Yeah, those are some of the ones I skipped when I selected the easiest/biggest-bang subset here. I was hoping to get most of the value without putting too much effort in, and unfortunately these aren't all trivial to implement.

  • sorting, in particular within the :refer vector, the imported pacakges, and the class names inside an imported prefix.

cljfmt already sorts the imports (and requires) before this PR. So what's left is sorting the nested lists that appear inside require/import clauses.

Moving code around is hard because of comments. Every time you find a comment, you need to use heuristics to decide what bit of the AST it should "attach" to and move it alongside that subtree.

There's another thorny issue with sorting the sublists: how do you handle line wrapping?

(ns foo
  (:require
    [p :refer [charlie delta
               foxtrot echo
               bravo alfa]]))

; After sorting, does this become...

; A:

(ns foo
  (:require
    [p :refer [alfa bravo charlie delta echo foxtrot]]))

; B:

(ns foo
  (:require
    [p :refer [alfa
               bravo
               charlie
               delta
               echo
               foxtrot]]))

; C:

(ns foo
  (:require
    [p :refer [alfa bravo
               charlie delta
               echo foxtrot]]))

(A) is not really okay since the line might end up arbitrarily long. I like (B) (and wrapping unconditionally), but I suspect that other people don't like that style so much, especially for shorter lines. If we go with (C), how does it work? Try to match the line length of the original or something?

And then whatever we choose to do with line wrapping has interactions with the comment handling as well.

I hope this gives some insight into the difficulties here. (By comparison, the things in this PR are simple, unambiguous rules that were easy to implement.)

If you have suggestions for a set of rules/heuristics that yield good results, are easy enough to describe, and aren't too hard to implement, I can look into adding them, but otherwise I'd prefer to punt on it for the moment.

  • :as before :refer

This raises a lot of the same questions as the above.

@cespare
Copy link
Owner Author

cespare commented Feb 16, 2021

@jwhitbeck I added a TODO to the code for implementing the other "How to ns" conventions. I'll interpret your thumbs-up as indicating that you're okay with merging in this change as-is.

@apesic this is ready for a code review (though if you have opinions about the formatting changes I'm interested in those too.)

@cespare
Copy link
Owner Author

cespare commented Feb 27, 2021

@apesic friendly ping

Copy link
Collaborator

@apesic apesic left a comment

Choose a reason for hiding this comment

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

Sorry for the delay--LGTM with one minor suggested addition.

format/testdata/require_before.clj Show resolved Hide resolved
This applies common NS style rules from "How to ns".

Fixes #68.
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.

Use lists instead of vectors for namespace :imports
3 participants