Attempt to implement comments parsing for deserialize #93
Conversation
This change attempts to handle the following case: "If a ; character is encountered outside of a string, that character and all subsequent characters to the next newline should be ignored." as described in https://github.com/edn-format/edn#comments
src/deserialize/parse.rs
Outdated
| @@ -397,6 +426,10 @@ fn read_map(chars: &mut std::iter::Enumerate<std::str::Chars>) -> Result<Edn, Er | |||
| loop { | |||
| match chars.next() { | |||
| Some((_, '}')) => return Ok(Edn::Map(Map::new(res))), | |||
| Some((_, ';')) => { | |||
| read_comment(chars)?; | |||
| () | |||
There was a problem hiding this comment.
You don't need () here (Rust will interpret a line ending in ; as ().
There was a problem hiding this comment.
I do think this expression is not necessary, as it is already being considered in parse
There was a problem hiding this comment.
thanks for the tip on that!, I'll fix all of those similar cases
the top level parse doesn't catch inside a map, vec, set, etc., so if i comment this line the following test fails (from line 852):
test deserialize::parse::test::parse_comment_inside_map ... FAILED
src/deserialize/parse.rs
Outdated
| @@ -13,6 +13,14 @@ pub(crate) fn parse( | |||
| Some((_, '(')) => read_list(chars)?, | |||
| Some((_, '#')) => tagged_or_set(chars)?, | |||
| Some((_, '{')) => read_map(chars)?, | |||
| Some((_, ';')) => { | |||
| let cmt = read_comment(chars)?; | |||
| match chars.next() { | |||
There was a problem hiding this comment.
Considering read_comment returns Edn::Empty and the functions receives a mutable chars, I don't think this match is necessary
There was a problem hiding this comment.
This is for the case where the only thing existing is a comment (tests starting on line 815) (fails without match). Normally the return from read_comment is ignored in favor of whatever the next call to parse returns. Your comment made me realize that an attempt to deserialize an empty or whitespace only string will fail instead of return Edn::Empty, so it would be surprising for comment to behave differently. For now I will make both return Edn::Empty, but the other option is to have both fail..
|
@evaporei Do you think we need a type |
|
@jollm I noticed the circleCI hasn't executed. You need to go to your fork circle CI and enable it for this project |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 84.47% 85.33% +0.86%
==========================================
Files 11 11
Lines 1977 2135 +158
==========================================
+ Hits 1670 1822 +152
- Misses 307 313 +6
Continue to review full report at Codecov.
|
- Return Edn::Empty for empty, whitespace only, and comment only strings. - Fix ; is implied () - Use compact lazy iterator expression to read comments - Add tests for empty and whitespace only cases - Add a test for literal whitespace ending comment instead of \n - Add test for string ending in multiple new lines
Update applicable comment test to call top level parse.
|
The latest diff is smaller and I hope cleaner. It avoids checking for comments and whitespace at inner levels. |
|
I promise to review again tomorrow |
|
Dont forget to enable your circle CI for this project https://app.circleci.com/pipelines/github/jollm/edn-rs |
|
"Commas , are also considered whitespace, other than within strings." from https://github.com/edn-format/edn#general-considerations This simplifies inner parsing levels, no longer having to check for commas.
This change attempts to handle the following case:
"If a ; character is encountered outside of a string, that character
and all subsequent characters to the next newline should be ignored."
as described in https://github.com/edn-format/edn#comments