Support for comments

[royaldark]

Hi, first of all thank you for making this awesome library! As someone still learning Rust, it's been a complete breeze to work with and I've learned a lot!

Looks like currently the parser tosses an unimplemented! when encountering a line starting with ;. Per the EDN spec,

"If a ; character is encountered outside of a string, that character and all subsequent characters to the next newline should be ignored.

What would be involved in adding support for comments? Since comments are line-level and exist independently of S-expressions, I guess support would have to be added to the handling of each existing EDN type? And maybe a new branch in the match to support comments which occur outside of any other form (e.g. beginning of file)?

[utkarshkukreti]

Comments are ignored by the official EDN parser right? What do you think about https://github.com/utkarshkukreti/edn.rs/commit/1556126e3f5b31b2e93877a21bfcece5220d9ca0? (I've only tested a couple of examples right now.)

[royaldark]

Ah, that looks simpler than I expected!

Seems to be working in many cases, but I've found at least one that causes a panic so far:

{;}
}

should parse to {}, but instead panics:

thread 'main' panicked at 'not yet implemented', C:\Users\joe\.cargo\git\checkouts\edn.rs-8371f6e65edfbdc7\1556126\src\parser.rs:270:18
stack backtrace:
   0: std::sys::windows::backtrace::unwind_backtrace
             at C:\projects\rust\src\libstd\sys\windows\backtrace\mod.rs:95
   1: std::sys_common::backtrace::print
             at C:\projects\rust\src\libstd\sys_common\backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at C:\projects\rust\src\libstd\panicking.rs:211
   3: std::panicking::default_hook
             at C:\projects\rust\src\libstd\panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at C:\projects\rust\src\libstd\panicking.rs:475
   5: std::panicking::begin_panic<str*>
             at C:\projects\rust\src\libstd\panicking.rs:409
   6: edn::parser::{{impl}}::read::{{closure}}
             at C:\Users\joe\.cargo\git\checkouts\edn.rs-8371f6e65edfbdc7\1556126\src\parser.rs:270
   7: core::option::Option<(usize, char)>::map<(usize, char),core::result::Result<edn::Value, edn::parser::Error>,closure>
             at C:\projects\rust\src\libcore\option.rs:414
   8: edn::parser::Parser::read
             at C:\Users\joe\.cargo\git\checkouts\edn.rs-8371f6e65edfbdc7\1556126\src\parser.rs:42
   9: edn::parser::{{impl}}::read::{{closure}}
             at C:\Users\joe\.cargo\git\checkouts\edn.rs-8371f6e65edfbdc7\1556126\src\parser.rs:195
  10: core::option::Option<(usize, char)>::map<(usize, char),core::result::Result<edn::Value, edn::parser::Error>,closure>
             at C:\projects\rust\src\libcore\option.rs:414
  11: edn::parser::Parser::read
             at C:\Users\joe\.cargo\git\checkouts\edn.rs-8371f6e65edfbdc7\1556126\src\parser.rs:42

Compared to the official parser:

user=> (clojure.edn/read-string "{}")
{}
user=> (clojure.edn/read-string "{;}")
RuntimeException EOF while reading  clojure.lang.Util.runtimeException (Util.java:221)

user=> (clojure.edn/read-string "{;}\n}")
{}

[royaldark]

As a side note, would you consider replacing panics in the parser with an Error value of some kind? I'm using this library to parse EDN files that are from an outside source, and therefore may be malformed. I'd like to be able to gracefully handle those parse errors when they occur, instead of the process simply dying.

[utkarshkukreti]

As a side note, would you consider replacing panics in the parser with an Error value of some kind?

Yes, definitely. Not sure why I went with panicking, but if I wrote this today, I'd never panic. I'd suggest replacing all the 4 unimplemented! to return an "unknown error" with whatever relevant span can be extracted. A test for each of the 4 cases would be wonderful! :)

[utkarshkukreti]

Please try out https://github.com/utkarshkukreti/edn.rs/commit/eaeaab8fbadaa0e39ce8e73f7ccc18e61f898e50.

Happy to accept PR for changing all panics to returning errors!

[royaldark]

eaeaab8 works for every test case I threw at it!