Skip to content

Fixes #1369: Add docstring to describe tests #1397

Merged
karlcow merged 2 commits into
webcompat:masterfrom
jeanhl:master
Mar 8, 2017
Merged

Fixes #1369: Add docstring to describe tests #1397
karlcow merged 2 commits into
webcompat:masterfrom
jeanhl:master

Conversation

@jeanhl

@jeanhl jeanhl commented Mar 6, 2017

Copy link
Copy Markdown
Contributor

I added docs to the unittests.

Edit:
r? @karlcow

Sorry for the mess and thanks for being so patient with me

@karlcow

karlcow commented Mar 7, 2017

Copy link
Copy Markdown
Member

@jeanhl I guess you are referring to #1369

first of all, Thanks! There are a couple of things to fit in the convention of this project.

See the format of the contribution done on #1392 and interactions with @miketaylr
There are a couple of things missing in your contribution: format and style. It is explained in
https://github.com/webcompat/webcompat.com/blob/master/CONTRIBUTING.md#submission-guidelines

  • The commit(s) should contain the issue number and be explicit about what they do.
  • The Pull Request should have a title starting with "Fixes #NNNN - "
  • The Pull Request should ask for a review (see top right for example. Reviewers). You can ask me or @miketaylr

@jeanhl jeanhl changed the title Issue 1369: Add comments describing tests in test_form.py Fixes #1369: Add docstring to describe tests Mar 7, 2017
@jeanhl jeanhl changed the title Fixes #1369: Add docstring to describe tests Fixes 1369: Add docstring to describe tests Mar 7, 2017
@jeanhl jeanhl changed the title Fixes 1369: Add docstring to describe tests Fixes #1369: Add docstring to describe tests Mar 7, 2017
@karlcow karlcow self-requested a review March 7, 2017 04:36
@karlcow

karlcow commented Mar 7, 2017

Copy link
Copy Markdown
Member

@jeanhl thanks for the pull request.

so to be really in harmony with the CONTRIBUTING guidelines, you need to change the commit message. Usually we do:

git commit -m '#1369 - Adds docstrings to form tests' tests/test_form.py

  • #1369 so the commit is automatically referenced in the issue.
  • Adds aka imperative voice instead of Added (this is more a question of style). Think about it as "This commit does something" instead "I did something".

I will review your commit.

@karlcow karlcow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you are not comfortable with rebasing. I'll do it.

Comment thread tests/test_form.py Outdated
class TestForm(unittest.TestCase):

def test_normalize_url(self):
''' Make sure that normalize_url method works.'''

This comment was marked as abuse.

@jeanhl

jeanhl commented Mar 7, 2017

Copy link
Copy Markdown
Contributor Author

Thank you! These are very helpful feedback. I'll revise the docstrings and do a new commit tomorrow morning. :)

@jeanhl

jeanhl commented Mar 7, 2017

Copy link
Copy Markdown
Contributor Author

I made the changes and committed them

@karlcow

karlcow commented Mar 7, 2017

Copy link
Copy Markdown
Member

Thanks a lot @jeanhl I will rebase it to have a cleaner commit.

@karlcow karlcow merged commit a07a163 into webcompat:master Mar 8, 2017
@jeanhl

jeanhl commented Mar 8, 2017

Copy link
Copy Markdown
Contributor Author

Thanks Karl! 😃

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.

3 participants