-
-
Notifications
You must be signed in to change notification settings - Fork 49
Update sphinx req to 1.7 and add version "policy" #88
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
==========================================
- Coverage 87.6% 87.15% -0.45%
==========================================
Files 5 5
Lines 734 693 -41
==========================================
- Hits 643 604 -39
+ Misses 91 89 -2
Continue to review full report at Codecov.
|
|
Python 2.7 with Sphinx 1.7 seems sensible! I've added a commit removing all the compatibility code we no longer need. |
sphinx_automodapi/automodsumm.py
Outdated
| if [clsonly, funconly, varonly].count(True) > 1: | ||
| self.warning('more than one of functions-only, classes-only, ' | ||
| self.warn('more than one of functions-only, classes-only, ' | ||
| 'or variables-only defined. Ignoring.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
| from . import cython_testpackage # noqa | ||
|
|
||
| __all__ = ['build_main', 'write_conf', 'run_sphinx_in_tmpdir'] | ||
| __all__ = ['write_conf', 'run_sphinx_in_tmpdir'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that OK to remove this from the namespace without a warning? I suppose yes as explicit imports still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only added this in that namespace in my last PR
| sphinx16: sphinx==1.6.7 | ||
| sphinx17: sphinx==1.7.9 | ||
| sphinx18: sphinx==1.8.5 | ||
| sphinx20: sphinx==2.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astrofrog - unrelated to this PR, but why do we pin the version so precisely? This case no newer bugfix releases get tested (surely, it's only relevant for the latest release). What about setting sphinx==2.1.* instead? (I couldn't comment on the next line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, does * work for pip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it does, we do that in ci-helpers, too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to do this in the commits I just pushed up since I'm not sure I fully understand the syntax here. So I'd say this could be a follow-on separate PR?
|
Please do add a changelog entry, too. |
|
OK, I think I addressed @bsipocz's suggetions (although I deferred 1 and ignored another - see comments above). |
This closes #85 by updating the sphinx dependecy to 1.7 as discussed further in that issue.
One thing I realized I wasn't sure about: when I dropped all the older tox environments, there was nothing left for py2.7, so I added one back in that does py2.7 and sphinx 1.7, which seems sensible. But I'm not 100% sure that one will work, so @astrofrog you might want to check that.