Skip to content

matrix.py: adds parameter to init function#587

Merged
ilya-khadykin merged 2 commits intoexercism:masterfrom
danishprakash:master
Oct 6, 2017
Merged

matrix.py: adds parameter to init function#587
ilya-khadykin merged 2 commits intoexercism:masterfrom
danishprakash:master

Conversation

@danishprakash
Copy link
Contributor

adds a parameter to init method making it easier for users to understand the problem.

Fixes #572

adds a parameter to init method making it easier for users to understand the problem.
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

I left a comment for discussion

@@ -1,3 +1,3 @@
class Matrix(object):
def __init__(self):
def __init__(self, matrix):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think matrix is somewhat confusing name, since it just a string of numbers divided by newlines.
So, it would be more appropriate to name it as matrix_string or something else.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, made the required change.

adds a parameter to init method making it easier for users to understand the problem.
@ilya-khadykin ilya-khadykin merged commit 6440df3 into exercism:master Oct 6, 2017
@ilya-khadykin
Copy link
Contributor

Thanks, @prakashdanish and congrats on your first contribution to the repo!

Please note that there are some special 'closing issue' keywords that you can put in description of your PR and Github will take care of closing the corresponding issue automatically after merge.
It makes our work much easier. I hope this hint will help you to make your future PRs a bit better

It's a good practice to squash your commits https://github.com/exercism/docs/blob/master/contributing/git-basics.md#squashing. It isn't a big deal since I can do it myself, but it would be awesome to just click on 'Merge pull request' button 😃

Just for future reference a guide of how to write good commit messages - https://chris.beams.io/posts/git-commit/#seven-rules

@danishprakash
Copy link
Contributor Author

danishprakash commented Oct 7, 2017

@m-a-ge Did you mean, I should've written something like matrix.py: fixes #572 in the shortlog or the body?
Will take care of squashing, thanks.

@ilya-khadykin
Copy link
Contributor

ilya-khadykin commented Oct 7, 2017

@prakashdanish yes, the full list of words: "close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved" followed by #issue_number. I prefer to put it in PR description rather than in commit message because it's easier that way to open respective issue if needed

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.

2 participants