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

spiral-matrix: implement. #526

Closed
wants to merge 6 commits into from
Closed

spiral-matrix: implement. #526

wants to merge 6 commits into from

Conversation

Average-user
Copy link
Member

@Average-user Average-user commented Sep 21, 2017

It is on the Unimplemented exercises list of Python in exercism.io. So, is better if it can stop being there.


Resolves #752

Copy link
Contributor

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!
I've left some minor comments. It's possible to create a separate issues for them if you want

"core": false,
"unlocked_by": null,
"difficulty": 2,
"topics": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add some topics of your choosing? You can use this list - exercism/problem-specifications#884 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not sure what to put. some idea ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Average-user I guess the following ones will be Ok:

loops
lists

algorithms is also suitable

@@ -0,0 +1,2 @@
def spiral():
Copy link
Contributor

Choose a reason for hiding this comment

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

It was reported here #509 that "placeholder" files should have expected parameters to help users start working on exercises quicker

@Average-user
Copy link
Member Author

I'm not sure about the topic, feel free to propose anything about them

Copy link
Contributor

@ilya-khadykin ilya-khadykin left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and sorry for the delay.

Could you please review my comments below?

import unittest

from spiral_matrix import spiral

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,2 @@
def spiral(n):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change n to something more meaningful like matrix_size?

@N-Parsons
Copy link
Contributor

@Average-user, are you still working on this?

@Average-user
Copy link
Member Author

I thought it was ready, I'll see whats left

@Average-user
Copy link
Member Author

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #784, the format of the canonical version comment should be as follows:

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

"difficulty": 2,
"topics": [
"algorithms",
"control-flow (loops)",
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

@N-Parsons N-Parsons Oct 28, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

You want me to put something between those two slashes, or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

Overall it looks good, but there are a few things that need changing before this can be merged (see my comments above).

@N-Parsons
Copy link
Contributor

@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.

@stale
Copy link

stale bot commented Dec 4, 2017

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Dec 4, 2017
@stale stale bot closed this Dec 11, 2017
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