Ensure printing/parsing edn strings is isomorphic #241
Conversation
| @@ -98,17 +98,17 @@ pub keyword -> Value = | |||
| } | |||
|
|
|||
| pub list -> Value = | |||
| "(" v:(value)* ")" { | |||
| "(" __ v:(value)* __ ")" { | |||
There was a problem hiding this comment.
Oh, interesting, I thought I'd covered this with
Line 128 in 90c1c59
edn/src/types.rs
Outdated
| @@ -83,8 +83,8 @@ impl Display for Value { | |||
| } | |||
| Map(ref v) => { | |||
| write!(f, "{{")?; | |||
| for (key, val) in v { | |||
| write!(f, " :{} {}", key, val)?; | |||
| for (index, (key, val)) in v.iter().enumerate() { | |||
There was a problem hiding this comment.
The leading colon is definitely wrong, so good fix there, but generally EDN does not include commas -- they're whitespace. Why would we include commas in Map but not the other container types? Because of the pairing? I'd prefer not to include them, which agrees with Clojure{Script?, although I could be argued down. @rnewman, do you have a strong opinion?
There was a problem hiding this comment.
Also, consider using join here, which is probably more efficient.
There was a problem hiding this comment.
I'll remove the commas
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
Signed-off-by: Victor Porof <[email protected]>
victorporof
force-pushed
the
edn-parse-parsed
branch
from
bba9349 to
4d83aaf
Compare
|
Addressed comments. |
| @@ -53,6 +53,7 @@ fn test_nil() { | |||
|
|
|||
| #[test] | |||
| fn test_nan() { | |||
| assert_eq!(nan("#fNaN").unwrap(), Float(OrderedFloat(f64::NAN))); | |||
There was a problem hiding this comment.
I don't believe this is correct.
#fNaN is itself a tag, so #fNaN should not match the nan production. Whitespace is required: #f NaN. This test should instead assert that #fNaN does not match nan, right?
@victorporof, am I misreading this change?
There was a problem hiding this comment.
good point. In that case, we should explicitly force the /required/ white space.
There was a problem hiding this comment.
@rnewman, makes sense. If that's the case, whitespace should be enforced.
Is that the case for #f +Infinity and #f -Infinity as well though? The reason I made the change in the first place was because #f +Infinity and #f -Infinity wasn't parseable. Wouldn't #f-Infinity have to also be parsed as a tag? Or are tags more picky with what characters are allowed?
There was a problem hiding this comment.
I've filed #244, let's move the conversation there?
Fixes #240