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

[#724] New practice exercise rational-numbers #755

Merged
merged 6 commits into from
Jun 12, 2021

Conversation

jiegillet
Copy link
Contributor

Here I go again!

@github-actions
Copy link
Contributor

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:

  • General steps

    • 🏆 Does this PR need to receive a label with a reputation modifier (reputation/contributed_code/{minor,major})? (A medium reputation amount is awarded by default, see docs)
  • Any exercise changed

    • 👤 Does the author of the PR need to be added as an author or contributor in <exercise>/.meta/config.json (see docs)?
    • 🔬 Do the analyzer and the analyzer comments exist for this exercise? Do they need to be changed?
    • 📜 Does the design file (<exercise>/.meta/design.md) need to be updated to document new implementation decisions?
  • Practice exercise changed

    • 🌲 Do prerequisites, practices, and difficulty in config.json need to be updated?
    • 🧑‍🏫 Are the changes in accordance with the community-wide problem specifiations?
  • Practice exercise tests changed

    • ⚪️ Are all tests except the first one skipped?
    • 📜 Does <exercise>/.meta/tests.toml need updating?

Automated comment created by PR Commenter 🤖.

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

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

Keep the PRs coming! 😂 ❤️

"basics",
"integers",
"floating-point-numbers",
"guards"
Copy link
Member

Choose a reason for hiding this comment

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

If the solution requires :math.pow, we have those options:

  1. Add prerequisite erlang-libraries, assuming the student will use :math.pow
  2. Add prerequisites that allow reimplementing :math.pow yourself, I'm guessing that would be either recursion or enum with ranges?
  3. Do nothing assuming most students will use Elixir 1.12 where Integer.pow and Float.pow are available

@neenjaw opinions? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think option 2 is the weakest, because Float.pow would be too hard to implement, people would search for it and find either :math.pow or the 1.12 functions.

I also think that not all students will have 1.12, that being said, one could understand basics as including basic math functions, so I don't really know which option to pick either.

Copy link
Contributor

Choose a reason for hiding this comment

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

The concern when Elixir was a lot smaller was that distro's didn't update their binaries very often. I think with Elixir's growing popularity distro , I'm personally of a stance not worrying too much about supporting legacy versions. I think options 1/3 are both reasonable. If we are looking at v3 in august, by then 1.12 will have been out for a few months making it much more reasonable to assume new comers would be using it

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's go with option 3. I'm convinced by:

If we are looking at v3 in august, by then 1.12 will have been out for a few months

For the record:

one could understand basics as including basic math functions

The way we defined concepts, basics includes using integer literals and basic math operators defined in the Kernel module (+, -, *, /), but using the Integer module is part of the integers concept 🙂.

end

describe "Exponentiation of a real number to a rational number" do
# We round float values to 10 decimal places to allow comparaison
Copy link
Member

Choose a reason for hiding this comment

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

Let's use assert_in_delta instead of rounding. It's the usual tool to use in unit tests for floats 🙂. I'm not certain if it makes any practical difference, but at least the code will be a bit shorter. Maybe we can put the delta value in a module attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I didn't know about assert_in_delta even though in hindsight of course it exists.

@@ -0,0 +1,59 @@
defmodule RationalNumbers do
@type rational :: {integer, integer}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@angelikatyborska angelikatyborska added the x:size/large Large amount of work label Jun 11, 2021
Copy link
Contributor

@neenjaw neenjaw left a comment

Choose a reason for hiding this comment

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

Nice work yet again!

@angelikatyborska
Copy link
Member

Closes #724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants