Closed bfontaine closed 6 years ago
@bfontaine My memory / context on this is fuzzy - this parses strings, right? What happens if there is a sentence and not a fraction, say "We can do X and/or Y"
- will this change safely ignore such strings?
@swaroopch No, fractions are literals like other numbers:
[1 1/2 1/3 1/4 1/5]
The only thing the parser might confuse them with are namespaced symbols like foo/bar
. I just did a couple tests: we correctly parse foo/bar
vs. 1/2
, but we accept foo/1
even if it’s forbidden in the spec: the part after /
is subject to the same restrictions as normal symbols, i.e. it can’t start with a digit.
Also, we shouldn’t accept spaces around /
: 1/2
should be parsed as Fraction(1,2)
but 1 / 2
should be parsed as 1 Symbol(/) 2
; a bit like 1.2
is parsed as a float while 1 . 2
is parsed as two integers and a symbol.
I’m going to add more tests to this PR to make sure we don’t break anything here.
I rewrote the implementation because I found a few issues when writing tests:
str(Fraction(2,1))
returns "2"
instead of "2/1"
1 / 2
was parsed as a fraction rather than an integer followed by a symbol followed by an integer/
wasn’t accepted as a symbol (the spec says that “/
by itself is a legal symbol”)We know take care of the fractions at the lexing step rather than the parsing one. This makes the last two issues trivial to fix because we don’t have to deal with the DIVIDE
symbol anymore.
We know take care of the fractions at the lexing step rather than the parsing one. This makes the last two issues trivial to fix because we don’t have to deal with the DIVIDE symbol anymore.
That's a great idea @bfontaine !
See https://github.com/edn-format/edn/issues/64.
This is the same PR as #23 but rebased on top of
master
.