Could some formatting decisions be based on sexpred nodes?
#333
Comments
|
Hello! I saw the PR for you from Ambrose, since I had one too involving unquote. I didn't imagine that yours would impact zprint, though. I didn't look closely at yours, though I thought it just applied to The error you have received comes from, unfortunately, deep in some really complex code trying to justify The short answer is that yes, with some "corrections", zprint will use the size of things that have been From looking at the expected and actual output, it doesn't look like a Expected output: Actual: Granted, this test does have an Can you tell me the extent of the change the PR makes? What things that were not previously qualified are qualified now? I'm still trying to figure out why anything getting |
|
This rewrite-clj PR would probably never have occurred to me, but the keen eye of @frenchy64 noticed that rewrite-clj was technically incorrect here, and I can't deny that he is right. 🙂 The PR is small and straightforward. To see the changes, have a peek at the diff. It's a bit hard for me to diagnose what should be happening in zprint, because I don't have a good understanding of what zprint is trying to do here. But it seems when lines get long we need to wrap, and when we need to wrap, there is vertical alignment going on. I have no clue how to replicate the failing test scenario from the cmd line, so I've just added the following to the bottom of (comment
(defn testing123[ f ]
(spit (str "out/" f)
(zprint-str (slurp (str "in/" f))
{:parse-string? true,
:fn-map {":require" [:none
{:list {:option-fn (partial
jrequireguide
:require)}}]}})))
(testing123 "lee1.clj")
(testing123 "lee2.clj")
:eoc)Without the PR, I think I can demonstrate the issue.
(ns lee1
(:require
[a.bb :refer [some long line that will need to wrap because it is indeed very long]]
[d.e :as f]))Produces (ns lee1
(:require [a.bb :refer [some long line that will need to wrap because it is
indeed very long]]
[d.e :as f]))Now let's introduce an unquote (ns lee2
(:require
[a.bb :refer [some long line that will need to wrap because it is indeed very long]]
[d.e :as f]
~`[x.y :as z]))And notice that we have extra indentation going on in (ns lee2
(:require [a.bb :refer [some long line that will need to wrap because it is
indeed very long]]
[d.e :as f]
~`[x.y :as z]))If we take a look at the column width, it matches the width of ;; 1234567
;; unquote
[d.e :as f]Coincidence? Could be, but I kinda doubt it! |
|
Thanks for pursuing this into zprint. That's above and beyond! A few things...
Thank you for running the zprint tests in rewrite-clj! And pursuing this into zprint along the way. I really appreciate it! It is also nice to hear from you again. |
|
I really can't comment on zprint's usage of For item 5, I don't know. But the effect is a bit of extra unwanted indentation from zprint, which is not great, but maybe not the end of the world for these perhaps peculiar use cases. For 6, thanks for the confirmation. I'll go ahead and merge the PR. It's always a pleasure to interact with you too @kkinnear. |
|
I think I might go ahead and temporarily disable the failing zprint test over a rewrite-clj to get a green build again. |
* Qualify @ ~ ~@ sexpr's under clojure.core z/sexpr should return the same value as clojure.core/read-string as per the documentation. The tests were moved from clojure.tools.reader.edn/read-string to clojure.tools.reader/read-string because the latter implements Clojure's reader. After this commit, the only special forms that return different values are ` and #=. * test & ci: bump sci-test for new tools.reader vars * lib test: patch zprint to disable new failing test See kkinnear/zprint#333 --------- Co-authored-by: lread <[email protected]>
Hi @kkinnear, I hope all is well with you!
While reviewing PR clj-commons/rewrite-clj#306 for rewrite-clj, I noticed that a single zprint test failed when running zprint 1.2.9 tests against the changes in the rewrite-clj PR.
Here's the failing test:
I dug around in zprint for a while, and my current best guess is that zprint is (at least sometimes) measuring column widths against
sexpred nodes.As you know, a
sexpred node does not necessarily match the length of the node in the original source.With the new PR we are testing zprint against, the width of certain
sexpred nodes is a wider, which might have helped to uncover the (potential) bug in zprint.Before rewrite-clj PR:
After rewrite-clj PR clojure vars are qualified:
What do you think? Could I be right about my assumption?
The text was updated successfully, but these errors were encountered: