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

New D210 check for white spaces. #95

Merged
merged 2 commits into from Jan 19, 2015
Merged

Conversation

@jirikuncar
Copy link
Contributor

@jirikuncar jirikuncar commented Jan 12, 2015

This is a proposal for checking whitespaces in docstrings.

  • number
  • error message
@Nurdok
Copy link
Member

@Nurdok Nurdok commented Jan 13, 2015

First of all, thank you so much for investing in this. It's greatly appreciated.

I'm currently on my phone, so I only skimmed the changes, but I have a few issues:

  • Why did you choose the D290 error code? Always use the next available one in the appropriate category.
  • The check name should be more specific. The entire D2xx category is about whitespace issues, so it's not enough to call it that.

@Nurdok
Copy link
Member

@Nurdok Nurdok commented Jan 13, 2015

You should also update the README file with the new error.

@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 13, 2015

  • number

Why did you choose the D290 error code? Always use the next available one in the appropriate category.

I'm not sure about the number and error message. Do you have any proposal?

@jirikuncar jirikuncar force-pushed the D290-whitespaces branch from e9765f5 to dbb7716 Jan 13, 2015
@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 13, 2015

@Nurdok based on README, I've changed the error code to 210. Is it ok?

@jirikuncar jirikuncar changed the title New D290 check for white spaces. New D210 check for white spaces. Jan 13, 2015
@Nurdok
Copy link
Member

@Nurdok Nurdok commented Jan 17, 2015

D210 is okay, but please also address my other comments.

@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 17, 2015

Can you please provide the message?

lines = eval(docstring).split('\n')
if lines[0].startswith(' ') or \
len(lines) == 1 and lines[0].endswith(' '):
return Error(
Copy link
Member

@Nurdok Nurdok Jan 17, 2015

Choose a reason for hiding this comment

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

Put the line break in the string.

return Error('D210: White spaces around docstring should '
             'be trimmed.')

@jirikuncar jirikuncar force-pushed the D290-whitespaces branch 2 times, most recently from f9b497a to 4f07e0a Jan 19, 2015
@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 19, 2015

@Nurdok thanks for your comments and time. :shipit:

@jirikuncar jirikuncar force-pushed the D290-whitespaces branch from 4f07e0a to 96d5839 Jan 19, 2015
@@ -778,6 +778,16 @@ def check_newline_after_last_paragraph(self, definition, docstring):
'quotes on separate line')

@check_for(Definition)
def check_surrounding_whitespaces(self, definition, docstring):
"""D210: White spaces around doctring should be trimmed."""
Copy link
Member

@Nurdok Nurdok Jan 19, 2015

Choose a reason for hiding this comment

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

You should update the error message here as well (you updated only the README) and in the tests.

Copy link
Contributor Author

@jirikuncar jirikuncar Jan 19, 2015

Choose a reason for hiding this comment

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

Ups. Sorry about it :(

@Nurdok
Copy link
Member

@Nurdok Nurdok commented Jan 19, 2015

I left one more comment in the actual code - but notice that the build is now failing on your branch. Make sure your tests pass.

@jirikuncar jirikuncar force-pushed the D290-whitespaces branch from 96d5839 to 7dad478 Jan 19, 2015
@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 19, 2015

@Nurdok please note that I've also added sudo: false to .travis.yml in order to use container based infrastructure (see http://docs.travis-ci.com/user/workers/container-based-infrastructure/).

lines = eval(docstring).split('\n')
if lines[0].startswith(' ') or \
len(lines) == 1 and lines[0].endswith(' '):
return Error("D210: No whitespaces allowed "
Copy link
Member

@Nurdok Nurdok Jan 19, 2015

Choose a reason for hiding this comment

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

This indentation looks wrong.

Copy link
Contributor Author

@jirikuncar jirikuncar Jan 19, 2015

Choose a reason for hiding this comment

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

Aaaah, broken :autoindent :(

@jirikuncar jirikuncar force-pushed the D290-whitespaces branch from 3e38a8b to dcd60de Jan 19, 2015
jirikuncar added 2 commits Jan 19, 2015
Signed-off-by: Jiri Kuncar <jiri.kuncar@cern.ch>
Reviewed-by: Amir Rachum <nurdok@gmail.com>
Signed-off-by: Jiri Kuncar <jiri.kuncar@cern.ch>
@jirikuncar
Copy link
Contributor Author

@jirikuncar jirikuncar commented Jan 19, 2015

@Nurdok sorry for so many iterations. Please let me know when you release it on PyPI so I can remove it from my package https://github.com/jirikuncar/invenio-kwalitee/blob/master/invenio_kwalitee/kwalitee.py#L82-L93. Thanks

Nurdok added a commit that referenced this issue Jan 19, 2015
New D210 check for white spaces.
@Nurdok Nurdok merged commit 8307b73 into PyCQA:master Jan 19, 2015
1 check passed
@Nurdok
Copy link
Member

@Nurdok Nurdok commented Jan 19, 2015

@jirikuncar Merged. It will probably take a few weeks before I make another release. In any case, thank you so much for investing the time and effort into this. I really can't thank you enough!

@jirikuncar jirikuncar deleted the D290-whitespaces branch Jan 19, 2015
@Nurdok Nurdok added this to the Next Release milestone Mar 14, 2015
@Nurdok Nurdok added this to the Next Release milestone Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants