Misc fixes #300
Misc fixes #300
Conversation
There was a problem hiding this comment.
While I like and regularly use the newly proposed newline style, a significant % of people don't, so I'd suggest keeping the traditional behavior and making the new one opt-in.
I know of teams that use refactor-nrepl as their official 'formatter' (that is, for ns forms).
WDYT?
Do people write the other way around on purpose when they are manually managing the ns form? I believe that this way of formatting dominates the other one completely, because you greatly reduce the risk of having the ns form take up too much horizontal space. Does the other way of formatting have any redeeming qualities beyond being familiar?
I really want the default behavior of the pretty printer to be in line with the style guide. I'd be way more open to adding a setting to explicitly preserve the old behavior. While I have someone on the line that gives a shit about ns formatting, another thing that's been bugging me about the current pretty printer is this: (:require
[this.very.long.namespace.for.gets.printed.very.weirdly
:as
long-ns]I believe this would be better as: (:require
[this.very.long.namespace.for.gets.printed.very.weirdly
:as long-ns]but didn't have the time to fix this today. This happens because we delegate the printing of the vector to I have some other stuff I want to fix (e.g. hotload dependency) so maybe we can batch up the contentious stuff and do a 3.0.0 release where some breakage can be permitted. |
I'd be inclined to say that judging the merits of each approach is besides the point. Instead one can simply ohserve that the current approach has been there for many years, and also aligns with the way that many people have written Clojure historically. I'd concur in that there's no big reason or advantage for the traditional style. It's just familiar, and saves one RET when typing it out. Aside that by now it's hardwired into the brains of a significant % of developers. This is to say: not introducing breaking changes is valuable. RH's talks have increased the sensibility in that direction, so ideally we wouldn't disappoint people. Especially when refactor-nrepl has gradually some traction over the years; I'd say that for re-gaining it we should be careful with our users. Finally, CIDER actually has some soft commitment to not further break behavior, see e.g. clojure-emacs/cider#2960
Worth pointing out that the styleguide is actually worded quite leniently for this specific point, surely in acknowledgement that this was a contentious point. |
Just in case, I assume you're familiar with clojure-emacs/orchard#103 and related issues/PRs? The whole goal appeared to be tricky - it could not work at all, or cause multiple bugs that were hard to track down, etc. |
Definitely agree with the sentiment, but that conversation was in the context of libraries / APIs. I don't think we'll break any existing programs with this change, though we might surprise people a little. The Along this line of reasoning comes the argument that would shouldn't put the most sensible defaults behind opt-in settings, but instead allow ourselves some breakage. I'm really glad you chimed in. I still want to do this, but I can definitely see the value in doing it in a 3.0.0 release and adding an explicit changelog entry. Do you think that's enough @vemv or should I also add a new setting so people can get the old behavior back? |
|
It might not be the most pertinent point, but I manually "correct" these newlines every single time. I'm quite familiar with Emacs' M-j (join-line) after these years. |
The humanity! 😱 I've just left it as is, with a niggling feeling of it being 'incomplete'. Letting the anger and resentment against my own creation grow and grow 👿 |
The content of the artifact cache was stored in a sorted map and then written to disk. The default readeres for edn does not contain a reader capable of handling sorted maps. This caused the artifact cache to stop working. This commits drops the use of sorted maps, as we never rely on them being sorted anyway.
This makes the pretty-printed namespace form more in line with the
clojure style guide.
Specifically we go from:
```
(ns foo.bar
(:require [this.library :as lib]))
```
To:
```
(ns foo.bar
(:require
[this.library :as lib]))
```
This commit closes clojure-emacs/clj-refactor.el#459
|
Aight, bumped the next release to 3.0.0 and added a proper changelog entry for this 'fix' :) |
Thanks! 3.0.0 is nice - often other projects follow semver more laxly. Opt-out seems ok, would be the second best choice imo 😄 a Java system property tends to be nice for these cases - this way one can set it in a team's project.clj, removing the need to worry about individuals' emacs setups. Would also be nice to keep the test coverage for the old formatting. It might be easy (without duplicating files such as |
|
I'll take a closer look at the proposed changes tomorrow/over the weekend. |
|
While I welcome the suggested change to the ns formatting, I'm thinking it'd be nice if we added some configuration flag that controls on the client-side whether there's a newline or not. That's trivial to do (just a |
Anyone willing to take that on? If not, I'm inclined to just drop the commit. There are many other bugs in this project that I'd rather spend that hour fixing. |
|
I can do this down the road myself, but I've been crazy busy at work for the past few months. I guess we can just open a ticket about this and hope that someone will get to doing it. |
I should have also read the discussion so far - seems you already discussed this. We can also "cheat" a bit and use a Java property to control this. Anyways, not a big deal for me. |
|
No issue 👍 I'll add the toggle in a PR. I happen to have the time and was planning to contribute to refactor-nrepl! |
|
I've deployed a new snapshot. |
@vemv Great! I just noticed you can also add this as part of https://github.com/clojure-emacs/refactor-nrepl#configuration I never used |
Could I trouble you to merge this and cut a snapshot release, @bbatsov?
Currently on a Windows machine and mranderson doesn't work here so I can't install locally or push to clojars. Haven't had time to investigate why, but would like to dogfood these changes prior to a new point release.