Skip to content

Implement exercise dominoes - #726

Merged
N-Parsons merged 11 commits into
exercism:masterfrom
cmccandless:implement-dominoes
Nov 5, 2017
Merged

Implement exercise dominoes#726
N-Parsons merged 11 commits into
exercism:masterfrom
cmccandless:implement-dominoes

Conversation

@cmccandless

@cmccandlesscmccandless commented Oct 6, 2017

Copy link
Copy Markdown
Contributor

TODO:

  • Add dominoes to config.json
  • Write test cases
  • Write example solution

Resolves#749

@cmccandlesscmccandless changed the title (WIP) Implement exercise dominoesImplement exercise dominoesOct 15, 2017
Comment threadexercises/dominoes/dominoes_test.py Outdated
from dominoes import chain


# test cases adapted from `x-common//canonical-data.json` @ version: 1.0.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a new format for the version string to reflect the name-change from x-common to problem-specifications that happened a while back (#784).

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I actually noticed that these tests weren't even based off of 1.0.0; it was the current version, 2.0.0. This has been updated.

Comment threadexercises/dominoes/README.md Outdated
which don't have a neighbour (the first and last stone) match each other.

For example given the stones `21`, `23` and `13` you should compute something
like `12 23 31` or `32 21 13` or `13 32 21` etc, where the first and last numbers are the same.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that they are specified like two digit numbers when these numbers are supposed to be separate things - I definitely prefer your implementation with tuples. Can you update the README to use tuples instead? It would be worth submitting this change to problem-specifications as well.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Comment threadexercises/dominoes/README.md Outdated

Some test cases may use duplicate stones in a chain solution, assume that multiple Domino sets are being used.

### Submitting Exercises

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you move this heading to level 2 (##) to save me doing it in #950?

@ilya-khadykin
ilya-khadykin removed the request for review from behrtamOctober 26, 2017 19:19
@cmccandless

Copy link
Copy Markdown
ContributorAuthor

@N-Parsons I've addressed your review comments and applied the latest changes to the description.

@N-Parsons

Copy link
Copy Markdown
Contributor

Sorry for taking a while to get back to this, @cmccandless!

@N-Parsons
N-Parsons merged commit 1b9ed09 into exercism:masterNov 5, 2017
@cmccandless
cmccandless deleted the implement-dominoes branch November 6, 2017 17:33
smalley pushed a commit to smalley/python that referenced this pull request Nov 12, 2017
* add dominoes/README.md
* Add test cases and example solution for dominoes
* add dominoes to config.json
* dominoes: add check for name == "__main__"
* dominoes: update canonical data version and formatting fixes in README
* dominoes: update README to latest description
RE: exercism/problem-specifications#972
smalley pushed a commit to smalley/python that referenced this pull request Nov 12, 2017
* add dominoes/README.md
* Add test cases and example solution for dominoes
* add dominoes to config.json
* dominoes: add check for name == "__main__"
* dominoes: update canonical data version and formatting fixes in README
* dominoes: update README to latest description
RE: exercism/problem-specifications#972
Sign up for freeto 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.

3 participants

@cmccandless@N-Parsons@ilya-khadykin