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

ns form: add linebreak after :refer-clojure #88

Open
imrekoszo opened this issue Sep 9, 2024 · 6 comments
Open

ns form: add linebreak after :refer-clojure #88

imrekoszo opened this issue Sep 9, 2024 · 6 comments
Labels
namespace related to ns form parsing or printing v1 blocker Required for a v1.0.0 release

Comments

@imrekoszo
Copy link
Contributor

imrekoszo commented Sep 9, 2024

Every clause but :refer-clojure is currently (@chrisoakman/[email protected]) followed by a linebreak, resulting in a somewhat inconsistent output:

(ns com.example.my-application.server
  "Example application HTTP server and routing."
  (:refer-clojure :exclude [atom count deref distinct empty first])
  (:require-macros
    [cljs.pprint :as pp])
  (:require
    [ring.util.response :as response])
  (:import
    (org.apache.commons.io FileUtils))
  (:gen-class
    :init ctor
    :constructors undefined))

I suggest adding a linebreak there too:

(ns com.example.my-application.server
  "Example application HTTP server and routing."
  (:refer-clojure
   :exclude [atom count deref distinct empty first])
  ...)
@oakmac
Copy link
Owner

oakmac commented Sep 10, 2024

This is a good point; I will give it a think.

When I was looking at existing code examples for :refer-clojure I saw a lot of cases where it was one line and it seemed like a regression in order to force it to two. But maybe the consistency and the visual indentation nesting is more important here.

@imrekoszo
Copy link
Contributor Author

I would vote for consistency especially that :refer-clojure also has other filters like :rename and :only

@NoahTheDuke
Copy link

lol if we're voting, I vote against splitting :refer-clojure across multiple lines. I've never seen it formatted that way.

In terms of existing code, grep.app has 3900 results for (:refer-clojure ... and 40 results for (:refer-clojure\n .... 100x more is a pretty strong indicator of existing community preference.

Maybe the heuristic here is only split across lines when there's more than :exclude.

@oakmac
Copy link
Owner

oakmac commented Sep 11, 2024

In terms of existing code, grep.app has 3900 results for (:refer-clojure ... and 40 results for (:refer-clojure\n .... 100x more is a pretty strong indicator of existing community preference.

IMO this is a pretty compelling argument for keeping the current behavior:

  • use a single line for :refer-clojure when there is only one key
  • split to multiple lines when multiple keys are present

@imrekoszo
Copy link
Contributor Author

  • use a single line for :refer-clojure when there is only one key
  • split to multiple lines when multiple keys are present

Random thought this morning: would this be a good default for all the other keys as well? I currently always put a newline after require and import, but there are devs who prefer to be compact when possible.

@oakmac
Copy link
Owner

oakmac commented Sep 20, 2024

Random thought this morning: would this be a good default for all the other keys as well? I currently always put a newline after require and import, but there are devs who prefer to be compact when possible.

I think there should always be a newline after :require, even with only one child form. I can possibly see shortening :import to one line when there is only one child? Always using a newline means that adding additional :require or :import children to one of these forms will result in a single-line diff per item, which I think is a nice property to have in this context.

But I think this is one of those cases where once the formatter is adopted and all ns forms are consistent, it won't matter much either way.

@oakmac oakmac added namespace related to ns form parsing or printing v1 blocker Required for a v1.0.0 release labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
namespace related to ns form parsing or printing v1 blocker Required for a v1.0.0 release
Projects
None yet
Development

No branches or pull requests

3 participants