The exceptions throw when parsing fails could be much more specific and helpful.

Description

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.

Environment

Both cljs and clojure.
OS independent.

Activity

Show:
Nicola Mometto
June 15, 2017, 4:54 PM

Haven't had time to take a deep look at this yet, will do soon now that 1.0.0 has been released

Nicola Mometto
July 2, 2017, 4:08 PM

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.

import
July 3, 2017, 2:47 PM

Comment made by: russolsen

Nicola,

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
yourself.

Nicola Mometto
July 3, 2017, 3:00 PM

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.

Nicola Mometto
July 11, 2017, 11:17 AM

Merged into master

Completed

Assignee

Nicola Mometto

Reporter

import

Labels

Approval

None

Patch

Code and Test

Priority

Major