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

Fix AllYourBase to follow problem specs #1370

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

pedrosnk
Copy link
Contributor

For All your base exercise, we need it's valid to return input or output error when both are provided. This commit allows both to be returned from the tested function.

Because we are expecting an error to be either input or output, I'm using a regex on the tests to match any of the errors to be valid.

@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 (x:size/{tiny,small,medium,large,massive})? (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.

Thank you for bringing this to my attention! Actually according to the Exercism-wide problem specifications, the error should be "input base must be >= 2" in case both input and output bases are negative (see https://github.com/exercism/problem-specifications/blob/main/exercises/all-your-base/canonical-data.json#L251-L263).

Could you modify your PR to follow that expectation?

@pedrosnk
Copy link
Contributor Author

TIL about the problem-specifications repo. In fact I don't believe there's much to change on the the tests if the specification must be followed.

Would make sense to update the elixir instructions following the error specification?

@angelikatyborska
Copy link
Member

angelikatyborska commented Sep 19, 2023

Would make sense to update the elixir instructions following the error specification?

You mean the file exercises/practice/all-your-base/.docs/instructions.md? That file is copied from problem specifications word for word and cannot be changed.

What we need is a single word change in the test file (compared to before your initial changes), to change the error message from output to input, and then a change in the file exercises/practice/all-your-base/.meta/example.ex so that the example solution passes the changed tests.

@pedrosnk
Copy link
Contributor Author

I follow you now, sorry, I thought it was already correct.

Thanks for walking me through it. Just patched the correct fix.

On the problem specification for this exercise, we should follow what is
defined. Matching the input error first in case both are invalid.

For All your base exercise, we need it's valid to return input or output error when both are provided.
This commit allows both to be returned from the tested function
@pedrosnk
Copy link
Contributor Author

test was failing because exercises/practice/all-your-base/.meta/example.ex needed to be updated. Just update it and rerun bin/ci.sh on my local machine. Seems to be passing now.

@angelikatyborska angelikatyborska changed the title Update all_your_base_test.exs Fix AllYourBase to follow problem specs Sep 20, 2023
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.

Thank you!

@angelikatyborska angelikatyborska merged commit 4ed55be into exercism:main Sep 20, 2023
11 checks passed
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.

2 participants