Skip to content

Added Rational Numbers exercise#1193

Merged
ErikSchierboom merged 2 commits intoexercism:mainfrom
tofische:rational-numbers-exercise
Jan 30, 2024
Merged

Added Rational Numbers exercise#1193
ErikSchierboom merged 2 commits intoexercism:mainfrom
tofische:rational-numbers-exercise

Conversation

@tofische
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown

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.

@github-actions github-actions Bot closed this Jan 17, 2024
@MatthijsBlom
Copy link
Copy Markdown
Contributor

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.

Comment thread exercises/practice/rational-numbers/src/RationalNumbers.hs
Comment thread exercises/practice/rational-numbers/src/RationalNumbers.hs Outdated
Comment thread exercises/practice/rational-numbers/src/RationalNumbers.hs
Comment thread exercises/practice/rational-numbers/test/Tests.hs Outdated
@ErikSchierboom
Copy link
Copy Markdown
Member

@MatthijsBlom Ping me when you've approved!

@MatthijsBlom
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

@MatthijsBlom could you review again?

@tofische
Copy link
Copy Markdown
Contributor Author

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

@MatthijsBlom
Copy link
Copy Markdown
Contributor

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

@tofische
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM! @MatthijsBlom If you find time and find things to improve, we can do that as a follow-up PR.

@ErikSchierboom ErikSchierboom merged commit 050fe8b into exercism:main Jan 30, 2024
tofische added a commit to tofische/haskell that referenced this pull request Jan 30, 2024
* Added Rational Numbers exercise

* Incorporated review comments from exercism#1193
tofische added a commit to tofische/haskell that referenced this pull request Jan 30, 2024
* Added Rational Numbers exercise

* Incorporated review comments from exercism#1193
tofische added a commit to tofische/haskell that referenced this pull request Jan 31, 2024
* Added Rational Numbers exercise

* Incorporated review comments from exercism#1193
tofische added a commit to tofische/haskell that referenced this pull request Jan 31, 2024
tofische added a commit to tofische/haskell that referenced this pull request Feb 1, 2024
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.

3 participants