Visitar URL original
Encode doubles as `Double`, do bounds checks, and add support for NaN and Infinity values by drvirgilio · Pull Request #667 · dhall-lang/dhall-haskell · GitHub
Skip to content

Encode doubles as Double, do bounds checks, and add support for NaN and Infinity values - #667

Merged
Gabriella439 merged 22 commits into
dhall-lang:masterfrom
drvirgilio:double
Nov 20, 2018
Merged

Encode doubles as Double, do bounds checks, and add support for NaN and Infinity values#667
Gabriella439 merged 22 commits into
dhall-lang:masterfrom
drvirgilio:double

Conversation

@drvirgilio

@drvirgilio drvirgilio commented Oct 31, 2018

Copy link
Copy Markdown

See discussion in dhall-lang issue #252. I'm not sure exactly on where the parser for "[-]Infinity" should go so I kind of guessed.

Gabriella439
Gabriella439 approved these changes Nov 1, 2018
View reviewed changes

@Gabriella439 Gabriella439 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'll merge this once the upstream change to the standard is merged

Comment thread dhall/src/Dhall.hs Resolved
@drvirgilio

drvirgilio commented Nov 8, 2018

Copy link
Copy Markdown
Author

I merged the changes to the standard so I think this is ready to be merged.

f-f
f-f approved these changes Nov 8, 2018
View reviewed changes

@f-f f-f left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drvirgilio LGTM 👍
Could we add some tests that exercise the new keywords and conversions?

PierreR
PierreR reviewed Nov 13, 2018
View reviewed changes
Comment thread dhall/src/Dhall/Pretty/Internal.hs Resolved
@Gabriella439

Gabriella439 commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator

@f-f: I'll go ahead and merge this now and add tests later, because I need to sync this repository with the dhall-lang test suite first. The other reason why is that I would like to add the tests to dhall-lang first and then mirror them to this repository rather than the other way around

@Gabriella439

Gabriella439 commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator

Actually, there are test failures on this branch that are being hidden by a CI misconfiguration on my part. I'm going to fix that first in a separate pull request and then fix the tests on this branch

@Gabriella439

Gabriella439 commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator

I have a pull request up against this branch with fixes to the test suite: drvirgilio#1

@drvirgilio

drvirgilio commented Nov 20, 2018

Copy link
Copy Markdown
Author

Finally got around to making a few changes:

  1. Added tests which check a few boundary conditions such as the point where Integer/toDouble rounds up to Infinity.
  2. Found a workaround for fromInteger's weird rounding behavior by using read . show.
  3. Made the changes suggested by @Gabriel439

@Gabriella439
Gabriella439 merged commit adf94a6 into dhall-lang:master Nov 20, 2018
@Gabriella439

Gabriella439 commented Nov 20, 2018

Copy link
Copy Markdown
Collaborator

@drvirgilio: Thanks for doing this! 🙂

@f-f f-f mentioned this pull request Dec 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gabriella439 Gabriella439 Gabriella439 approved these changes
@f-f f-f f-f approved these changes
+1 more reviewer
@PierreR PierreR PierreR left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@drvirgilio @Gabriella439 @PierreR @f-f