The exceptions thrown when edn/clojure code parsing fails are less helpful than they might be:
The same message is used in slightly different circumstance. For example "EOF while reading" is used in three slightly different situations in clojure/tools/reader.clj, while "EOF while reading character" is used four times in the same file.
The exception messages frequently do not include much of the available context. Which token is bad? Which map contains an odd number of items?
The line number where the error occurred is not always returned.
Both cljs and clojure.
Haven't had time to take a deep look at this yet, will do soon now that 1.0.0 has been released
Hi , first of all, many thanks for the work you've put into this.
I finally had a chance to look at this, generally, I like the improvements and I think I'm likely to include this in some future release, relatively soon.
A few issues I'd like to see addressed before that happens:
as a stylistic implementaion detail, I'd rather see keyword passed around rather than strings in e.g. `read-delimited` or `read-token`, and have a dispatch table in the errors namespace.
some error messages seem to have lost starting column details, like in the case of `throw-eof-delimited`. Please reinstate that, we want improvements in error messages to be strictly additive
move both .errors and .inspect under the .impl namespace segment, we do not want to expose those as public API
we need to incorporate the fix for https://github.com/clojure/tools.reader/commit/becf3abdc39bd6420582615ee3b9c428077f30d8 in this patch set
exclude from this patch whiteline/newline changes, commented code, variable renames (e.g. reader to rdr), formatting changes (e.g. in the ns declaration for edn.cljs) and non-related improvements (e.g. switching from string-push-back-reader to indexing-push-back-reader for read-string)to keep the changeset as contained as possible. If you fee like any of those changes are valid and wish to see them included, please open separate tickets for them.
I'd like to see the cljs and clj implementations match as much as possible. Is there a reason for having different implementations of e.g. reader-error between clj and cljs? If there is, fine, otherwise please use the same implementation
please remove the println at the top of edn.clj
Please do let me know if you don't have time to handle the points I've reaised and I'll do them myself. In the meantime, thanks again for your work on this.
Comment made by: russolsen
Really glad you are finding the patch helpful. Philosophically I have no problems with the changes
you are suggesting - this whole thing has been one of those "pull on a thread and before you know
it the whole sweater is gone" kind of exercises.
Unfortunately I find myself really busy at the moment, so it's probably better if you run with it
Hi Russ, no worries, that's completely understandable.
Thanks again for all the effort you've put into this, I'll try to get this into tools.reader by the end of the week.
Merged into master