Close #305: Qualify @ ~ ~@ sexpr's under clojure.core #306
Conversation
frenchy64
force-pushed
the
issue-305-sexpr-differs-from-read-string
branch
2 times, most recently
from
ea99838 to
56dcbe5
Compare
frenchy64
force-pushed
the
issue-305-sexpr-differs-from-read-string
branch
3 times, most recently
from
03e847f to
548773b
Compare
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 #=.
frenchy64
force-pushed
the
issue-305-sexpr-differs-from-read-string
branch
from
548773b to
37b75f0
Compare
| [clojure.test :refer [deftest is]] | ||
| [clojure.tools.reader.edn :refer [read-string]] | ||
| [clojure.test :refer [deftest is testing]] | ||
| [clojure.tools.reader :as rdr] |
There was a problem hiding this comment.
Do you need to use clojure.tools.reader instead of clojure.tools.reader.edn for your tests to be valid?
There was a problem hiding this comment.
My initial motivation to tweak this was to automatically test this statement in the docs: "Within reason, Clojure’s read-string and rewrite-clj’s sexpr functions should return equivalent Clojure forms".
So that meant using clojure.core/read-string instead of clojure.tools.reader.edn/read-string. This is because:
- I interpreted "Clojure’s read-string" to mean
clojure.core/read-string - Things like
'@~are not valid edn, so the tests would not make sense.
Since CLJS doesn't have read-string, I replaced c.t.n with c.t.n.edn for reason number 2.
The main downside is *read-eval* can be dangerous, so I tried to be as conservative as possible in setting that.
I think perhaps the test in t-parsing-seqs could also test against clojure.core/read-string in the :default case for reason number 1.
There was a problem hiding this comment.
Ah, ok, thanks for explaining, @frenchy64.
The thing is sci tests are currently failing.
Sci tests are a bit involved, would you like me to have a look-see?
|
Go ahead and have a look and I can jump in if needed.
…-------- Original Message --------
On 9/6/24 19:30, Lee Read wrote:
@lread commented on this pull request.
---------------------------------------------------------------
In [test/rewrite_clj/parser_test.cljc](#306 (comment)):
> (:require [clojure.string :as string]
- [clojure.test :refer [deftest is]]
- [clojure.tools.reader.edn :refer [read-string]]
+ [clojure.test :refer [deftest is testing]]
+ [clojure.tools.reader :as rdr]
Ah, ok, thanks for explaining, ***@***.***(https://github.com/frenchy64).
The thing is sci tests are currently failing.
Sci tests are a bit involved, would you like me to have a look-see?
—
Reply to this email directly, [view it on GitHub](#306 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AACGFJHKYQQMR6JIOA2O7RDZVJCLPAVCNFSM6AAAAABMQVEVIWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBXGI4TCOJSGM).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
For compatibility with new tests in clj-commons/rewrite-clj#306, add `clojure.tools-reader`: - `read-string` - `*read-eval*`
…fers-from-read-string * upstream/main: dev & ci: bump deps (clj-commons#312) maint: bump tools.reader (clj-commons#310) dev & ci: bump deps (clj-commons#309) publish: apply version 1.1.48 doc: maint - contributors and used by (clj-commons#308) dev & ci: bump deps (clj-commons#307)
|
Ok @frenchy64, I've resolved the failing sci test by exposing But now I've just noticed that this PR breaks a zprint test expectation (we run tests of all popular libs using rewrite-clj against rewrite-clj changes). So we'll have to look into why that is so. |
|
Hmm... the zprint test is failing due to differences only in line wrapping and formatting. |
|
I've taken a peek a zprint and see some evidence of it basing formatting decisions on |
|
@frenchy64, I'm going to go ahead and finally merge this puppy. |
Equivalent code in Edamame.
https://github.com/borkdude/edamame/blob/23490ab3b810ced710c233d6cabd09c32a8c9a9d/src/edamame/impl/parser.cljc#L613-L665