- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 UnboundLocalError when building changes (1.8) #6135
Conversation
Codecov Report
@@ Coverage Diff @@
## 1.8 #6135 +/- ##
=========================================
+ Coverage 82.14% 82.25% +0.1%
=========================================
Files 307 308 +1
Lines 40663 40678 +15
Branches 6285 6284 -1
=========================================
+ Hits 33403 33459 +56
+ Misses 5872 5832 -40
+ Partials 1388 1387 -1
Continue to review full report at Codecov.
|
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.
LGTM with nits
.gitignore
Outdated
@@ -2,6 +2,7 @@ | |||
*.egg | |||
*.so | |||
*.swp | |||
*,cover |
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 feel this is not needed.
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 ran:
tox -e coverage
coverage annotate
Which gave me a pile of these mixed in with my changes, but I guess maybe it's more useful to be able to git clean -f
without -x
.
.gitignore
Outdated
@@ -2,7 +2,6 @@ | |||
*.egg | |||
*.so | |||
*.swp | |||
*,cover |
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, sorry. I don't know about "coverage annotate".
tests/test_build_changes.py
Outdated
@@ -34,10 +34,10 @@ def test_build(app): | |||
|
|||
|
|||
@pytest.mark.sphinx( | |||
'changes', testroot='changes', srcdir='changes-none', |
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.
Thank you for explanation. Indeed, this is needed.
@tk0miya I think this can land in current state? Would be good to improve the testing later but that can be a separate change. |
@bz2 I think the |
Split testing of changes builder to its own test file and root. Improve coverage a little, and add case that fails if a module directive is included in the rst source. Correctly handle changesets with a module declared. Fixes sphinx-doc#6134
@tk0miya Amended. Would you like me to also put up a branch for master, or is your process merging forwards from the 1.8 branch anyway? |
Thank you for updating! I'm merging this into 1.8 branch. And it will be merged into master branch later. |
Split testing of changes builder to its own test file and root. Improve coverage a little, and add case that fails if a module directive is included in the rst source.
Correctly handle changesets with a module declared.
Ignore coverage annotations files in git.Fixes #6134 for the 1.8 branch.
Same change basically applies for master, can propose that separately or whatever the process is for landing on multiple branches.