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

alphametics: v1.3.0 Add test for two digits carry #1344

Merged
merged 1 commit into from
Oct 4, 2018
Merged

alphametics: v1.3.0 Add test for two digits carry #1344

merged 1 commit into from
Oct 4, 2018

Conversation

omer-g
Copy link
Contributor

@omer-g omer-g commented Oct 1, 2018

Add test where result is longer than largest addendum by more than one
digit. In solutions that sum each digit separately this could be useful
to make sure final carry is handled correctly. For example: not
treated as a single digit.

Resolves #1342

Add test where result is longer than largest addendum by more than one
digit. In solutions that sum each digit separately this could be useful
to make sure final carry is handled correctly. For example: not
treated as a single digit.

Resolves: 1342
@Insti
Copy link
Contributor

Insti commented Oct 1, 2018

Awesome, thanks @omer-g .
This sounds reasonable, and the PR looks good to me.

I'm a but rusty on the mechanics of alphametics, so it would be nice if @petertseng could run his test script over it to double check.

@petertseng
Copy link
Member

if @petertseng could run his test script over it to double check.

that is not possible for hours or days (unknown period of time). I suggest that interested parties should run it themselves

@omer-g
Copy link
Contributor Author

omer-g commented Oct 2, 2018

Thank you. I checked the new test case manually on several solutions in the Python track, including the example.py solution. I can try and run the script (if I'll know where to find it).

@Insti
Copy link
Contributor

Insti commented Oct 2, 2018

@omer-g
Copy link
Contributor Author

omer-g commented Oct 2, 2018

Thanks. I ran the script and the tests passed.

@Insti
Copy link
Contributor

Insti commented Oct 2, 2018

Great, thanks.
One of the other maintainers will look over this and merge it sometime over the next few days.

@cmccandless
Copy link
Contributor

@Insti @petertseng

Probably worth raising another issue for this (and I will if needed), but has there ever been discussion on creating a GitHub Status Check that runs @petertseng's verify scripts?

@petertseng
Copy link
Member

discussion on creating a GitHub Status Check that runs @petertseng's verify scripts?

#460

feel free to add any additional information there if you have any

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

clearly a useful case, with a clear description of why it is useful.

it's been > 72 hours since submission; there have been no requests for changes and there are (now including myself) two Approvals (in the GitHub sense of the word) so it seems safe to merge it.

Thanks for adding the case!

@petertseng petertseng merged commit 361cf3c into exercism:master Oct 4, 2018
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.

4 participants