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

Refactor - split to modules, add parser tests, update readme #200

Merged
merged 21 commits into from Sep 11, 2016

Conversation

Projects
None yet
2 participants
@Nurdok
Copy link
Member

Nurdok commented Aug 2, 2016

@sigmavirus24 If you have some time, I'd love it if you could give this a look.
Also, this fixes #196.

tox
pathlib

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 2, 2016

Member

So, I have a quibble with this file. Is tox a test dependency and the rest are dependencies enumerated in tox, or are these test dependencies and tox is a different kind of requirement?

This comment has been minimized.

@Nurdok

Nurdok Aug 4, 2016

Member

Separated to tests.txt and test_env.txt.

src/main.py Outdated
@@ -0,0 +1,19 @@
#! /usr/bin/env python

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 2, 2016

Member

Is this file in the right place?

This comment has been minimized.

@Nurdok

Nurdok Aug 4, 2016

Member

See my comment in setup.py.

setup.py Outdated
@@ -27,12 +29,13 @@
'License :: OSI Approved :: MIT License',
],
keywords='pydocstyle, PEP 257, pep257, PEP 8, pep8, docstrings',
packages=('pydocstyle',),
py_modules=('main',),

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 2, 2016

Member

Why are you packaging main?

This comment has been minimized.

@Nurdok

Nurdok Aug 4, 2016

Member

The point of this is to allow to run pydocstyle from the command line without the entry point (e.g., if you want to just unzip it somewhere and run it without installing).

This comment has been minimized.

@sigmavirus24

sigmavirus24 Aug 5, 2016

Member

Have you investigated how this is actually installed on a person's computer though? I think this might not do exactly what you want.

This comment has been minimized.

@Nurdok

Nurdok Aug 8, 2016

Member

I'll do some experiments to make sure.

Nurdok added some commits Sep 6, 2016

Merge branch 'master' into splitfiles
# Conflicts:
#	src/pydocstyle.py
#	src/pydocstyle/tests/test_decorators.py
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 6, 2016

@sigmavirus24 care to take another look? I fixed the issue with the main module and also had to do a manual merge from #203 .

Example
-------
>>> check(['pydocstyle.py'], checked_codes=['D100'])

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 6, 2016

Member

FYI, checked_codes will no longer work

This comment has been minimized.

@Nurdok

Nurdok Sep 7, 2016

Member

Updated the docstring.

count = 0
for error in errors:
sys.stderr.write('%s\n' % error)
code = ReturnCode.violations_found

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 7, 2016

Member

Is it necessary to reassign this for every error? Why not do something like:

count = 0
for error in errors:
    print('%s' % error)   # because we all agree sys.stderr isn't correct
    count += 1
if count > 0:
   code = ReturnCode.violations_found
else:
   code = ReturnCode.no_violations_found

This comment has been minimized.

@Nurdok

Nurdok Sep 7, 2016

Member

Fixed.

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Sep 7, 2016

Sorry for the lag in review @Nurdok this is a huge change, and my own personal life is a bit hectic and will be so for a couple weeks. So I'm reviewing this in chunks of free time as I find them. =)


count = 0
for error in errors:
sys.stderr.write('%s\n' % error)

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 7, 2016

Member

print instead of sys.stderr?

This comment has been minimized.

@Nurdok

Nurdok Sep 7, 2016

Member

Since this is a change in behavior, I'd rather do this in a separate PR from this refactoring.

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 7, 2016

Member

That sounds perfectly reasonable to me :)

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Sep 7, 2016

Some questions too:

  • I see you have moved the tests into src/pycodestyle. Do you then plan to ship those tests so people can run them on their system after install? Is this for convenience? If so, what convenience?
  • You have pycodestyle/main.py that reminds me that people can (probably, I haven't tested this) currently do python -m pycodestyle [options] file.py to invoke pycodestyle. As you have this structured now, they cannot do that. Do you wish to continue supporting that? (To do so, you need a pycodestyle/__main__.py that does pretty much exactly the same thing as pycodestyle/main.py.
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 7, 2016

No problem about the lag - I appreciate the review and it's not urgent.

  • Regarding test location - I did this sort of on instinct. I guess putting the test next to pydocstyle would be more convenient. I'll go ahead and do that.
  • Regarding main - I actually didn't know about __main__.py. I'll read about it and change it to support python -m pydocstyle *.

Thanks!

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Sep 7, 2016

@Nurdok having the tests in the package is perfectly fine too. I didn't mean to convince you to undo that. I'm just curious as to why you did it.

The reason I ask is because I have seen the benefit of always importing the fully-qualified module after it's been installed in the virtualenv via tox. This is why Flake8 has src/flake8 and tests/ so that the tests are easily editable (because I'm super lazy) and it relies on flake8 being installed prior to running the tests. The layout is personal preference.

The reason I bring this up is because I think I saw relative imports in the tests and that concerns me a bit. If you're not testing what gets installed into a virtualenv, you're not testing what a user will receive after running pip install pydocstyle. Does that make sense?

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 7, 2016

I realize the test location is a preference, but as I read your suggestion it immediately became clear to me that it would solve all the ugly relative imports, so I'll take it :)

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Sep 7, 2016

Hah, cool. =D

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 7, 2016

Do you see a reason not to put tests in src? i.e., have pydocstyle and tests siblings in the src directory. It will still have the benefits you describe, since the tests directory will not be installed in the virtualenv.

@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Sep 7, 2016

Nope. =)

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 8, 2016

I completed both points, although __main__ does not work for Python 2.6.

@Nurdok Nurdok merged commit d22428e into PyCQA:master Sep 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Sep 11, 2016

@sigmavirus24 I merged this so that other work on pydocstyle can continue. If you have further comments, I'll be happy to hear them.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment