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

Don't unwrap CLJC lib specs when cleaning a namespace #266

Open
danielcompton opened this issue Sep 25, 2019 · 4 comments
Open

Don't unwrap CLJC lib specs when cleaning a namespace #266

danielcompton opened this issue Sep 25, 2019 · 4 comments

Comments

@danielcompton
Copy link

danielcompton commented Sep 25, 2019

We have a namespace like this:

(ns my.app.ns
  (:require [vlad.core :as vlad :refer [attr chain join present Validation]]
            #? (:cljs [goog.date.Interval :as Interval])
            [clojure.string :as string]
            [cemerick.url :as url]))

after running cljr-clean-ns, it fully expands the clj and cljs versions to:

(ns my.app.ns
  #?@
   (:clj
    [(:require
      [cemerick.url :as url]
      [clojure.string :as string]
      [vlad.core :as vlad :refer [attr chain join present Validation]])]
    :cljs
    [(:require
      [cemerick.url :as url]
      [clojure.string :as string]
      [goog.date.Interval :as Interval]
      [vlad.core :as vlad :refer [attr chain join present Validation]])]))

This namespace form is twice as long, and (IMO) obscures which parts of the namespace differ between clj and cljs. To tell the difference, you need to check each line and find the corresponding one in the other branch.

Would you consider a feature request to not fully expand CLJC forms in the :require?

I'd argue this should be the default behaviour, but could also see it being an option if people liked this kind of expansion. The new feature wouldn't automatically convert fully expanded CLJC forms back, it would just preserve what was already there.

@magnars
Copy link
Contributor

magnars commented Sep 26, 2019

I would be very happy to see a change like this, if possible. I always presumed this behavior was because it would be too complicated otherwise.

@expez
Copy link
Member

expez commented Sep 26, 2019

Would you consider a feature request to not fully expand CLJC forms in the :require?

👍

I'd argue this should be the default behaviour [...]

I think so too.

@yannvanhalewyn
Copy link
Contributor

+1 have the same issue

@vemv
Copy link
Member

vemv commented Jun 29, 2021

FWIW, https://github.com/gfredericks/how-to-ns has a fairly fine-grained support for reader conditionals

Starting with 3.0.0, refactor-nrepl will be closer to 'how-to-ns' style so mixing and matching different tooling wouldn't even be particularly impactful.

Lately I was thinking that internally, refactor-nrepl could have a protocol for its ns formatting. Our current refactor-nrepl.ns.pprint ns could back the default impl, but other impls could be how-to-ns among others (a few libs in this space have popped up in the last few years)

Obviously in an ideal world we'd just have one formatter to rule them all, but like anything, that would need significant work.

@vemv vemv added this to Low prio in refactor-nrepl Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
refactor-nrepl
Low prio / PR welcome
Development

No branches or pull requests

5 participants