spiral-matrix: implement.#526
spiral-matrix: implement.#526Average-user wants to merge 6 commits intoexercism:masterfrom Average-user:master
Conversation
ilya-khadykin
left a comment
There was a problem hiding this comment.
Thanks for working on this!
I've left some minor comments. It's possible to create a separate issues for them if you want
| "slug": "spiral-matrix", | ||
| "core": false, | ||
| "unlocked_by": null, | ||
| "difficulty": 2, |
There was a problem hiding this comment.
Could you please add some topics of your choosing? You can use this list - exercism/problem-specifications#884 (comment)
There was a problem hiding this comment.
i'm not sure what to put. some idea ?
There was a problem hiding this comment.
| @@ -0,0 +1,2 @@ | |||
| def spiral(): | |||
There was a problem hiding this comment.
It was reported here #509 that "placeholder" files should have expected parameters to help users start working on exercises quicker
|
I'm not sure about the topic, feel free to propose anything about them |
ilya-khadykin
left a comment
There was a problem hiding this comment.
Thanks for your patience and sorry for the delay.
Could you please review my comments below?
| import unittest | ||
|
|
||
| from spiral_matrix import spiral | ||
|
|
There was a problem hiding this comment.
Could you add a comment that this tests are based on version 1.0.0 of https://github.com/exercism/problem-specifications/blob/master/exercises/spiral-matrix/canonical-data.json?
| @@ -0,0 +1,2 @@ | |||
| def spiral(n): | |||
There was a problem hiding this comment.
Could you please change n to something more meaningful like matrix_size?
|
@Average-user, are you still working on this? |
|
I thought it was ready, I'll see whats left |
|
@m-a-ge I think there is nothing left to do. Right? |
| @@ -5,6 +5,7 @@ | |||
| # Tests are based on the version 1.0.0 of: | |||
| # https://github.com/exercism/problem-specifications/blob/master/exercises/spiral-matrix/canonical-data.json | |||
There was a problem hiding this comment.
Per #784, the format of the canonical version comment should be as follows:
# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0| "unlocked_by": null, | ||
| "difficulty": 2, | ||
| "topics": [ | ||
| "algorithms", |
There was a problem hiding this comment.
control-flow (loops) should just be loops - see TOPICS.txt for a full list of valid topics.
| 11 16 15 6 | ||
| 10 9 8 7 | ||
| ``` | ||
| ## Source |
There was a problem hiding this comment.
There should be a blank line between sections, and there should actually be a ## Submitting exercises section here - check another exercise for the wording, or regenerate this README using the configlet.
To use the configlet (on Linux/OSX, not sure about commands for Windows):
$ cd exercism/python # or wherever you have this repo cloned to
$ bin/fetch_configlet
$ bin/configlet generate . --only spiral-matrix| from spiral_matrix import spiral | ||
|
|
||
| # Tests are based on the version 1.0.0 of: | ||
| # https://github.com/exercism/problem-specifications/blob/master/exercises/spiral-matrix/canonical-data.json |
There was a problem hiding this comment.
This comment should be # Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0 as per #784. Could you also move it down by a line? (ie. two blank lines before, one after)
There was a problem hiding this comment.
You want me to put something between those two slashes, or not?
There was a problem hiding this comment.
@Average-user, no, it's just a shorthand way of saying that we are skipping some uninteresting directories in the middle. The full path would be problem-specifications/exercises/spiral-matrix/canonical-data.json, which is a bit long to include in a comment, and the middle directories are fairly obvious.
N-Parsons
left a comment
There was a problem hiding this comment.
Overall it looks good, but there are a few things that need changing before this can be merged (see my comments above).
|
@Average-user, are you still working on this? There are just a few minor changes that need to be made before this can be merged. |
|
This issue has been automatically marked as |
It is on the Unimplemented exercises list of Python in exercism.io. So, is better if it can stop being there.
Resolves #752