search

exercism / haskell

Exercism exercises in Haskell.
https://exercism.org/tracks/haskell
MIT License
493 stars 191 forks source link

Added Rational Numbers exercise #1193

Closed tofische closed 8 months ago

tofische commented 8 months ago

Added a new Haskell practice exercise Rational Numbers based on the available problem specification.

github-actions[bot] commented 8 months ago

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

MatthijsBlom commented 8 months ago

I don't think reduce should be exported. Going by the instructions there shouldn't be a need for that either:

Your implementation of rational numbers should always be reduced to lowest terms.

I understand this to mean that it should be impossible for a user of this module to construct unreduced fractions.

To still be able to test proper reduction, we could require export of projections numerator and denominator.

ErikSchierboom commented 8 months ago

@MatthijsBlom Ping me when you've approved!

MatthijsBlom commented 8 months ago

We now have rational :: (a, a) -> Rational a, but this allows creating fractions with 0 as denominator. Seemingly, having rational :: (a, a) -> Maybe (Rational a) would be more idiomatic. However, then div would still allow us to commit division by zero, so I think we might as well keep the present rational :: (a, a) -> Rational a.

tofische commented 8 months ago

I don't think reduce should be exported. Going by the instructions there shouldn't be a need for that either:

Your implementation of rational numbers should always be reduced to lowest terms.

I understand this to mean that it should be impossible for a user of this module to construct unreduced fractions.

To still be able to test proper reduction, we could require export of projections numerator and denominator.

You are correct, I'll update the exercise.

ErikSchierboom commented 8 months ago

@MatthijsBlom could you review again?

tofische commented 8 months ago

I'm now a little bit confused - shall I do something in order to enable this PR to be merged?

MatthijsBlom commented 8 months ago

Sorry, I haven't been able to find the energy to review properly again.

tofische commented 8 months ago

Sorry, I haven't been able to find the energy to review properly again.

No problem, I was just unsure because one check (although not required) failed.

ErikSchierboom commented 8 months ago

@MatthijsBlom if you haven't found the time in the next couple of days, I'll look into it