Skip to content

fix nth-prime typespec#1634

Open
Cohen-Carlisle wants to merge 1 commit into
exercism:mainfrom
Cohen-Carlisle:fix-nth-prime-typespec
Open

fix nth-prime typespec#1634
Cohen-Carlisle wants to merge 1 commit into
exercism:mainfrom
Cohen-Carlisle:fix-nth-prime-typespec

Conversation

@Cohen-Carlisle

Copy link
Copy Markdown
Member

I noticed that it uses non_neg_integer() but pos_integer() would be more appropriate (for both the count param and the returned prime number).

@github-actions

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

Automated comment created by PR Commenter 🤖.

@jiegillet

Copy link
Copy Markdown
Contributor

Hi!
I personally agree with you, pos_integer would be more appropriate
However there is a test that checks the output for the value 0, which means that 0 is a legitimate input.
However however, it just crashes on that value, so 0 is just as legitimate as -1 or "seventeen", so maybe 0 should not be reflected in the type.
However however however, the original test description prescribes returning the error message "there is no zeroth prime" which explicitly mentions 0, so maybe that should be a legitimate input, and the real fix for this would be to return an error tuple for 0, and keep crashing for -1 and "seventeen".

What a mess 😆
What do you think @Cohen-Carlisle?

@Cohen-Carlisle

Copy link
Copy Markdown
Member Author

Hmm, yeah that's an interesting point about the "there is no zeroth prime" error from the canonical data. The guidance says you don't necessarily need to check or generate an error message based on that string. So in this case I do think that changing to pos_integer() is appropriate for the input as well as the output. The other option I see is to use return values like {:ok, prime} and {:error, reason}, but it sees like overkill just to return a special error message for 0.

@jiegillet

jiegillet commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Yeah, I think we are on the same page.

Then here is what I would propose to do: we keep pos_integer() because it's appropriate, but also we remove the "there is no zeroth prime" test altogether and add comment about it in exercises/practice/nth-prime/.meta/tests.toml explaining why we didn't implement it (like we already do in exercises/practice/nucleotide-count/.meta/tests.toml), something like "information on valid input is encoded in the type".

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