Import & test fixes. #20
Conversation
|
Thanks for the PR. Taking the points in order:
|
|
I think 1) is reasonable (well I don't have a better idea right now). |
|
For 2: There's no need for mock to be a dependency except for in testing. Perhaps we could switch to having some requirements files instead of having everything in the setup file. I agree that the way setuptools handles these requirements is not ideal. For 3: Yes, there's no real reason to use nose. The coverage bit can be done by coverage itself. If we were to transition to pytest it would be to take advantage of its features. |
|
I don't think you can have conditional requirements in requirements files, so I'd rather keep the conditional import in |
- Try importing `sphinx.ext.napoleon` first (if you have a recent Sphinx installed, there's no reason to have `sphinxcontrib.napoleon` installed too). - Try importing `unittest.mock` first in the test suite (same as above). - Make tests runnable as `python test_defopt.py`. - Ignore vim's `.*.swp` swap files.
|
Force pushed, removed conditional import of sphinx. |
|
I added a commit that wraps lines at 79 characters. I understand not being strict about pep8 but I really have trouble working with files with long lines. |
|
FWIW markers in test_requires are only supported since setuptools 20.5 (https://setuptools.readthedocs.io/en/latest/history.html#id179) so I'd rather just get rid of test_require (and setup_requires) completely. edit: ... on the other hand even the extra_requires approach only worked since setuptools 18, so that's just 9 months worth of releases (18 to 20.4) that you'd gain for not using markers in test_requires :-) (https://hynek.me/articles/conditional-python-dependencies/) |
defopt.py
Outdated
| :param short: Dictionary mapping parameter names to letters to use as | ||
| alternative short flags. | ||
| :param short: Dictionary mapping parameter names, after conversion of | ||
| underscores to dashes, to letters, to use as alternative short flags. |
There was a problem hiding this comment.
Nice catch. I find this hard to parse with the commas - could you instead put the added bit in parentheses?
|
The tests are failing because enum34 wasn't installed, which means the environment markers you used didn't work. I vaguely remember having trouble trying to do this previously, which may be why I ended up doing it the way I did. You'll either have to figure out how to fix it, or undo that part of the change. For nosetests, it technically belongs in setup_requires while the Travis build is using |
Basically, you want `{"foo-bar": "f"}`, not `{"foo_bar":
"f"}`. (I would keep the current behavior, as it also allows
`{"no-foo-bar": "F"}`.)
|
Took me a few attempts but I finally got it right, I think. |
|
I don't think you have the coverage bit right. You need |
|
Also there's no reason to continue to depend on nose now. |
|
Done. |
|
Thanks! |
sphinx.ext.napoleonfirst (if you have a recent Sphinxinstalled, there's no reason to have
sphinxcontrib.napoleoninstalledtoo).
unittest.mockfirst in the test suite (same as above).python test_defopt.py..*.swpswap files.