Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle fractions #49

Merged
merged 1 commit into from
Sep 7, 2018
Merged

Conversation

bfontaine
Copy link
Collaborator

See edn-format/edn#64.

This is the same PR as #23 but rebased on top of master.

@swaroopch swaroopch mentioned this pull request Aug 23, 2018
@swaroopch
Copy link
Owner

swaroopch commented Aug 23, 2018

@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?

@bfontaine
Copy link
Collaborator Author

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

@bfontaine bfontaine force-pushed the feature/fractions branch 5 times, most recently from 0607ad5 to 901db6f Compare August 26, 2018 20:13
@bfontaine
Copy link
Collaborator Author

bfontaine commented Aug 26, 2018

I rewrote the implementation because I found a few issues when writing tests:

  • integer fractions were dumped as integers because 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.

@swaroopch
Copy link
Owner

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 !

@swaroopch swaroopch merged commit a91ad8a into swaroopch:master Sep 7, 2018
@bfontaine bfontaine deleted the feature/fractions branch September 7, 2018 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants