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

Fix pep257 errors #3

Merged
merged 4 commits into from Jul 28, 2012

Conversation

Projects
None yet
2 participants
@dpursehouse
Copy link
Contributor

commented Jun 20, 2012

In these commits I've removed all the pep257 violations from pep257.py and converted the test module to use the built-in python unit test library.

Still to do:

  • Make the test module pass pep8.
  • Add new unit tests to cover the errors removed from pep257.py
  • Add unit tests for the rest of the functionality

dpursehouse added some commits Jun 20, 2012

Correct spelling of "triple"
The word "triple" was misspelled as "tripple" in a few places.
Remove PEP-257 errors
The script has a number of PEP-257 violations which are being used
to demonstrate/test its functionality.  However this means it's
difficult to deploy the script in an environment where all Python
files, including the script itself, will be automatically checked
for PEP-257 compliance.

The errors are removed and will be added as separate tests.
@keleshev

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2012

It looks like we misunderstood each other a bit—I meant it would be great if you add tests that cover check_* functions functionality into test_pep257.py (which are now tested with invalid docstrings in code itself), not convert py.test-tests to unittest-tests.

I hate unittest framework with passion :-) and use py.test for all my python tests. So I will not pull 17fd8de74a0b218f0ee52e45c801f98d8daff1d1.

Otherwise it all looks good and I will pull other commits, when I get home.

@keleshev

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2012

Ok now I read your "Still to do" :-)

Glad that you are about to add those test, but anyway, py.test is what is used for this project. Try it, I'm sure you will love it.

@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2012

@halst Sorry, I missed that "py.test" is a unit test framework. I'll have a look into that and try to add some new tests.

I'm using the built-in unittest framework in my environment so I'll move the changes that I made into a separate file and maintain that separately on my fork.

@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2012

OK, I've cleaned this up a bit. Removed the unwanted commits related to unittest, and moved the other completely unrelated commits to separate pull requests.

I'll continue with this one later, adding py.test tests to replace the tests removed in 93b8308.

@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2012

I haven't forgotten about this; I'm just busy with other work. I'll try to make time for it in the next weeks.

@keleshev

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2012

@dpursehouse ok, good to know that you're on it.

dpursehouse added some commits Jul 19, 2012

Fix spelling error
In the method `check_blank_after_last_paragraph`, the word
`paragraph` was misspelled as `paragtaph`.
Add more test cases
More test cases are added to replace the test functionality that
was removed from the main module in commit 3773da5.

Also fixed a couple of PEP-8 warnings:  comparison to None should
be done with "is None" rather than "== None".
@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2012

This is almost done. There are still 2 test cases to be implemented - marked as TODO in test_pep257.py

@keleshev keleshev merged commit 48e5779 into PyCQA:master Jul 28, 2012

@keleshev

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2012

Sorry for taking so long.

I merged this, but I found something funny in case of:

r"""PEP257 Use \"\"\"triple double quotes\"\"\".

For consistency, always use \"\"\"triple double quotes\"\"\" around
docstrings. Use r\"\"\"raw triple double quotes\"\"\" if you use any
backslashes in your docstrings. For Unicode docstrings, use
u\"\"\"Unicode triple-quoted strings\"\"\".

"""

The problem with this is when you do:

>>> import pep257
>>> help(pep257.check_triple_double_quotes)

you get this:

Help on function check_triple_double_quotes in module pep257:

check_triple_double_quotes(docstring, context, is_script)
    PEP257 Use \"\"\"triple double quotes\"\"\".

    For consistency, always use \"\"\"triple double quotes\"\"\" around
    docstrings. Use r\"\"\"raw triple double quotes\"\"\" if you use any
    backslashes in your docstrings. For Unicode docstrings, use
    u\"\"\"Unicode triple-quoted strings\"\"\".

And I don't know how te get around of this without breaking PEP 257 rules.

@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2012

I didn't know about the help method before.

I guess you want it to display without the escaping on the quote characters? I don't see how that could be done without violating pep257. One way would be to remove the quote characters from the docstring:

r"""PEP257 Use triple double quotes.

For consistency, always use triple double quotes around
docstrings. Use raw triple double quotes if you use any
backslashes in your docstrings. For Unicode docstrings, use
Unicode triple-quoted strings.

"""
@keleshev

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2012

I wanted the quote characters in the error message to aid quick recognizability of errors. If we remove all occurrences of """, r""", u""", the error messages will be much less readable.

Do you think it will be suboptimal to use ''' instead for those docstrings?
Maybe pep257 itself should allow ''' in case the docstring has """ in it?

@dpursehouse

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2012

Right, it will be less readable if we remove them. If we want to keep them, and not use escaping, the only way to do it is to use '''.

How about making an update suggestion to pep257? I guess that would need to be sent to doc-sig at python.org according to the page:

http://www.python.org/dev/peps/pep-0257/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.