Skip to content

phone number: only one problem per test input#1959

Merged
SaschaMann merged 5 commits intoexercism:mainfrom
astrieanna:patch-1
Feb 11, 2022
Merged

phone number: only one problem per test input#1959
SaschaMann merged 5 commits intoexercism:mainfrom
astrieanna:patch-1

Conversation

@astrieanna
Copy link
Copy Markdown
Contributor

Because the area code is not allowed to start with 0 or 1, inputs designed to elicit other errors should not use area codes that start with either of those digits.

Because the area code is not allowed to start with 0 or 1, inputs designed to elicit other errors should not use area codes that start with either of those digits.
@astrieanna astrieanna changed the title [Phone Number] Only one problem per test input phone number: only one problem per test input Feb 10, 2022
Copy link
Copy Markdown
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

I do like this suggestion. What @astrieanna seems to be suggesting is that by having the other errors use a format that will also be caught by the first error, is that an incorrect implementation would actually be able to go through, or at least make it more difficult for a student to figure out where they went amiss.

@astrieanna As you can see the CI failed. That is because tests in this definiton are immutable. Instead, what you'll want to do, if you're up for it, is duplicate the test cases, generate new ids (You can use https://www.uuidgenerator.net/version4) and add reimplements as key with the old UUID.

This will indicate that a new test has replaced an old one. We usually use the comments field to indicate why.

Feel free to dismiss this review once you've made the changes.

@ErikSchierboom @petertseng your input is welcome.

@ErikSchierboom
Copy link
Copy Markdown
Member

This is a great idea!

As @SleeplessByte said, the test cases are immutable, so could you apply what @SleeplessByte suggested and create new, duplicated test cases with the input changed and their reimplements key set to the corresponding old test case's uuid value?

@astrieanna
Copy link
Copy Markdown
Contributor Author

astrieanna commented Feb 11, 2022

Thanks for interpreting my terse description correctly. 😅 My thought was that checking multiple errors at once both:

  1. Rejects some otherwise correct implementations by requiring a particular hierarchy of error messages.
  2. Gives confusing feedback to some broken implementations (since there's an "incorrect" error message would also be true description of the input).

I decided that one of the three I'd originally changed didn't make sense. In that test, the input is a 9-digit number that starts with a 1; the starting 1 could be interpreted as the country code, so there's no other error that would make sense to return instead of an error about too few digits.

I don't have the power to dismiss a review (or I can't find the button), so I've requested a re-review instead.

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!

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

Very good idea 👏

@SaschaMann SaschaMann merged commit f2a2c56 into exercism:main Feb 11, 2022
@SleeplessByte
Copy link
Copy Markdown
Member

It definitively was ok. Thanks everyone <3

ErikSchierboom added a commit to ErikSchierboom/problem-specifications that referenced this pull request Feb 11, 2022
* Format using prettier (exercism#1917)

Format using prettier

* updated description of anagrams exercise (exercism#1928)

* updated description of anagrams

* changed anagram description to be one-sentence-per-line

* updated description of anagrams to use sets

* Update Licence

Give a look at the discussion in BR exercism#1930

* rational-numbers: test to reduce abs value (exercism#1938)

* Change saddle point references to row, column (exercism#1948)

* word-search: Add test case

* Update exercises/word-search/canonical-data.json

Agreed.

Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

* meetup: improve descriptions by saying why each case is tested (exercism#1919)

descriptions show whether a date is the first, last, or an arbitrary
middle date of the week. This helps understand why certain cases are
selected.

Closes exercism#974

* word-search: Add cases checking for concatenation and wrapping

The author of this commit thinks that concatenation is highly unlikely,
but the wrapping might be useful to check in languages that allow
negative indices.

* `flatten-array` Add additional test cases (exercism#1953)

* Add additional test cases to flatten-array

* Update exercises/flatten-array/canonical-data.json

Co-authored-by: Peter Tseng <petertseng@users.noreply.github.com>

Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: Peter Tseng <petertseng@users.noreply.github.com>

* Fix bowling game copy (exercism#1955)

Fixes exercism#1954

* Add action to format code (exercism#1941)

* build(deps): bump DavidAnson/markdownlint-cli2-action (exercism#1952)

Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 5.0.0 to 5.1.0.
- [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@b3c3b40...744f913)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Reduced rational nr. should be in standard form. (exercism#1958)

* Reduced rational should be in standard form.

The current instructors fail to mention that a reduced rational number should always be rendered in standard form (without any negative value at the denominator).

* remove superflous blank lines; fix wording

* scale-generator: use flat and sharp symbols (exercism#1942)

* Update configlet part in README (exercism#1949)

Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>

* phone number: only one problem per test input (exercism#1959)

* [Phone Number] Only one problem per test input

Because the area code is not allowed to start with 0 or 1, inputs designed to elicit other errors should not use area codes that start with either of those digits.

* Respect immutability

* Correct field name: s/comment/comments/

* Comments should contain a list.

* Allow prettier to improve comments

* book-store: reorder keys

* darts: reorder keys

* grade-school: reorder keys

* hamming: reorder keys

* high-scores: reorder keys

* largest-series-product: reorder keys

* list-ops: reorder keys

* luhn: reorder keys

* triangle: reorder keys

* scale-generator: reorder keys

* saddle-points: reorder keys

* diffie-hellman: reorder keys

* collatz-conjecture: reorder keys

* anagram: reorder keys

* accumulate: reorder keys

* Add CI script to check correct order of keys

Co-authored-by: Bart Massey <bart.massey@gmail.com>
Co-authored-by: y8l <72172663+yusufadell@users.noreply.github.com>
Co-authored-by: Ivan Ivanov <mr.mstranger@gmail.com>
Co-authored-by: Damian C. Rossney <dcr8898@users.noreply.github.com>
Co-authored-by: mariohuq <mariohuq@github.com>
Co-authored-by: mariohuq <mario.huq@gmail.com>
Co-authored-by: Peter Tseng <petertseng@users.noreply.github.com>
Co-authored-by: Peter Tseng <pht24@cornell.edu>
Co-authored-by: AH WEI <88667293+kwchang0831@users.noreply.github.com>
Co-authored-by: BethanyG <BethanyG@users.noreply.github.com>
Co-authored-by: Cedd Burge <ceddlyburge@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Davide Alberto Molin <davide@davidemolin.com>
Co-authored-by: wolf99 <281700+wolf99@users.noreply.github.com>
Co-authored-by: June <12543047+junedev@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: Leah Hanson <astrieanna@gmail.com>
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.

5 participants