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

Added fromInt #261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added fromInt #261

wants to merge 1 commit into from

Conversation

reactormonk
Copy link

@reactormonk reactormonk commented Feb 25, 2021

Description of the change

Pasta from https://discourse.purescript.org/t/create-a-guide-for-porting-haskell-numbers-to-purescript-numbers/1261/2 as discussed with @hdgarrood on slack.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

testFromInt = do
assert "Zero" $ fromInt 0 == 0.0
assert "Negative" $ fromInt (-1) == (-1.0)
assert "Positive" $ fromInt 1 == 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add tests for 2 and 5 please? Just in case.

@@ -0,0 +1,12 @@
module Data.Ring.Extra where
Copy link
Contributor

Choose a reason for hiding this comment

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

From the outside, it makes little sense to put this function into Data.Ring.Extra rather than just putting it into Data.Ring. What if we move abs and signum from Data.Ord into Data.Ring? I think that would mean that Data.Ord no longer needs to import Data.Ring, which would allow us to use >= here.

I think we should basically inline the definitions of power and Additive too, since importing either of those in Data.Ring will also cause a cycle.

Copy link
Author

Choose a reason for hiding this comment

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

That'd be a breaking change though, if Data.Ord doesn't export abs/signum anymore.

Copy link
Contributor

@hdgarrood hdgarrood Feb 26, 2021

Choose a reason for hiding this comment

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

Correct, so this would have to wait until the next round of breaking changes. Even though it is breaking, I prefer it over adding a new Data.Ring.Extra module because:

  • the API surface makes more sense,
  • we'd end up wanting to merge Data.Ring.Extra back into Data.Ring at some point in the future, which would also be breaking, and
  • I think it makes more sense to put abs and signum into Data.Ring anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Ord also depends on negate in its usage of -1:

instance ordArray :: Ord a => Ord (Array a) where
  compare = \xs ys -> compare 0 (ordArrayImpl toDelta xs ys)
    where
    toDelta x y =
      case compare x y of
        EQ -> 0
        LT -> 1
        GT -> -1 -- here it is

@JordanMartinez JordanMartinez added the type: breaking change A change that requires a major version bump. label Jun 21, 2021
@JordanMartinez JordanMartinez added the purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 label Mar 16, 2022
@JordanMartinez
Copy link
Contributor

Since it's not clear to me how to resolve this comment #261 (comment) I'm going to say this won't be included in 0.15.0.

@JordanMartinez JordanMartinez removed the purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 label Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants