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

TODOs inside a :param: have a uri, but not a refid. #5800

Closed
wants to merge 1 commit into from

Conversation

daleevans
Copy link

@daleevans daleevans commented Dec 15, 2018

Subject: This avoids a KeyError when generating a link to a TODO that crashes sphinx, and gives a warning so the offending TODO can be tracked down.

Feature or Bugfix

  • Bugfix

Purpose

  • avoid a specific crash when building todolists

Detail

  • a todo inside a :param: will have a relative uri, but not a refid. instead of crashing, just skip adding the target to the link, and emit a warning that tells the user where the todo is so they can move it or ignore it. besides that, a missing refid in other cases will be easier to debug with this warning message rather than just a stack trace and a crash.

This avoids a KeyError when generating a link to a TODO that crashes sphinx, and gives a warning so the offending TODO can be tracked down.
@codecov
Copy link

codecov bot commented Dec 15, 2018

Codecov Report

Merging #5800 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5800      +/-   ##
==========================================
- Coverage   83.27%   83.27%   -0.01%     
==========================================
  Files         296      296              
  Lines       39579    39582       +3     
  Branches     5872     5872              
==========================================
+ Hits        32959    32960       +1     
- Misses       5260     5262       +2     
  Partials     1360     1360
Impacted Files Coverage Δ
sphinx/ext/todo.py 83.44% <50%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e96fe...87e25f4. Read the comment docs.

@tk0miya
Copy link
Member

tk0miya commented Dec 17, 2018

Could you share some example to reproduce the error please?
I think this does not resolve the problem itself. So I'd like to check it.

@tk0miya tk0miya added this to the 1.8.3 milestone Dec 17, 2018
@daleevans
Copy link
Author

Could you share some example to reproduce the error please?
I think this does not resolve the problem itself. So I'd like to check it.

Sure, if you clone this:

https://github.com/daleevans/sphinx-toc-bug-demo

and run make html in the sphinx directory you should get this:

Running Sphinx v1.8.2
making output directory...
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 3 source files that are out of date
updating environment: 3 added, 0 changed, 0 removed
reading sources... [100%] modules
/home/xxx/bug/sphinx-toc-bug-demo/module.py:docstring of module.Test.a:4: WARNING: Duplicate ID: "module-module".
looking for now-outdated files... none found
pickling environment... done
checking consistency... /home/xxx/bug/sphinx-toc-bug-demo/sphinx/source/modules.rst: WARNING: document isn't included in any toctree
done
preparing documents... done
writing output... [ 33%] index
Exception occurred:
  File "/home/xxx/venv/lib/python3.6/site-packages/docutils/nodes.py", line 567, in __getitem__
    return self.attributes[key]
KeyError: 'refid'
The full traceback has been saved in /tmp/sphinx-err-b96xdwu5.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Makefile:19: recipe for target 'html' failed
make: *** [html] Error 2

@tk0miya
Copy link
Member

tk0miya commented Dec 21, 2018

Thank you for example. I found the root cause of the error. docutils does not assign refid attributes to the target nodes under TextElement.
I just pushed #5848 to follow such case. I believe it fixes the error perfectly.
Could you check this please?

@daleevans
Copy link
Author

Thank you for example. I found the root cause of the error. docutils does not assign refid attributes to the target nodes under TextElement.
I just pushed #5848 to follow such case. I believe it fixes the error perfectly.
Could you check this please?

Yes, this works perfectly. Thanks!

tk0miya added a commit that referenced this pull request Dec 23, 2018
Fix #5800: todo: crashed if todo is defined in TextElement
@tk0miya
Copy link
Member

tk0miya commented Dec 23, 2018

Thank you for confirm. I just merged #5848 instead.
Thanks,

@tk0miya tk0miya closed this Dec 23, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants