Skip to content

Fixes #29942 -- Link docs to github source #16918

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

Fixes #29942 -- Link docs to github source #16918

wants to merge 9 commits into from

Conversation

ewjoachim
Copy link
Contributor

@ewjoachim ewjoachim commented Jun 1, 2023 •
edited
Loading

This was implemented during the DjangoCon Europe sprint, thanks to @sarahboyce and partly based on #16203 by @zachborboa

This fixes 2 issues at once:

  • Adding source links again in the docs. They used to be there and then didn't anymore. That's 29942. The way to fix that is to identify line numbers for function by reading AST only, instead of importing the whole function. This should work in 99% (guesstimate) of cases where Python code is writen as simple functions, classes and methods.
  • Links now link to GitHub, on the (hopefully) correct branch as discussed in the forum

This was referenced Jun 1, 2023
Closed
Closed
@ewjoachim ewjoachim force-pushed the github-link-docs-392-29942 branch from cbc86d7 to ffe6eb2 Compare June 1, 2023 14:18
@ewjoachim
Copy link
Contributor Author

ewjoachim commented Jun 1, 2023 •
edited
Loading

Added a temporary commit with partial fixes. I'll do a proper push later
EDIT: done :)

@ewjoachim ewjoachim force-pushed the github-link-docs-392-29942 branch from 0792a75 to 862069e Compare June 1, 2023 23:37
sarahboyce
sarahboyce reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ewjoachim! I think this is a much nicer solution ⭐
I have a couple of minor stylistic comments and there is a pesky windows test that needs a tweak but I like this a lot 👍

sarahboyce
sarahboyce approved these changes Jun 4, 2023
View reviewed changes
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thank you so much!
Note this will probably get another review before being accepted 👍

@ewjoachim
Copy link
Contributor Author

Thank you for suggesting working on this and for the help and the review :) Team work !

@smithdc1
Copy link
Member

Unfortunately this crashes on Windows.

reading sources... [ 15%] ref/class-based-views/generic-date-based
Traceback (most recent call last):
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\events.py", line 96, in emit
    results.append(listener.handler(self.app, *args))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\ext\linkcode.py", line 54, in doctree_read
    uri = resolve_target(domain, info)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\smith\projects\django\docs\_ext\github_links.py", line 124, in github_linkcode_resolve
    path, lineno = get_path_and_line(module=module, fullname=fullname)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\smith\projects\django\docs\_ext\github_links.py", line 68, in get_path_and_line
    locator = get_locator(path)
              ^^^^^^^^^^^^^^^^^
  File "C:\Users\smith\projects\django\docs\_ext\github_links.py", line 45, in get_locator
    file_contents = file.read_text()
                    ^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.11_3.11.1008.0_x64__qbz5n2kfra8p0\Lib\pathlib.py", line 1059, in read_text
    return f.read()
           ^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.11_3.11.1008.0_x64__qbz5n2kfra8p0\Lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 23159: character maps to <undefined>

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\cmd\build.py", line 284, in build_main
    app.build(args.force_all, args.filenames)
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\application.py", line 347, in build
    self.builder.build_update()
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\builders\__init__.py", line 311, in build_update
    self.build(to_build,
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\builders\__init__.py", line 327, in build
    updated_docnames = set(self.read())
                           ^^^^^^^^^^^
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\builders\__init__.py", line 434, in read
    self._read_serial(docnames)
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\builders\__init__.py", line 455, in _read_serial
    self.read_doc(docname)
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\builders\__init__.py", line 511, in read_doc
    publisher.publish()
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\docutils\core.py", line 226, in publish
    self.apply_transforms()
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\docutils\core.py", line 206, in apply_transforms
    self.document.transformer.apply_transforms()
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\transforms\__init__.py", line 80, in apply_transforms
    super().apply_transforms()
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\docutils\transforms\__init__.py", line 173, in apply_transforms
    transform.apply(**kwargs)
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\transforms\__init__.py", line 359, in apply
    self.app.emit('doctree-read', self.document)
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\application.py", line 458, in emit
    return self.events.emit(event, *args, allowed_exceptions=allowed_exceptions)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\Users\smith\projects\django\.venv\Lib\site-packages\sphinx\events.py", line 107, in emit
    raise ExtensionError(__("Handler %r for event %r threw an exception") %
sphinx.errors.ExtensionError: Handler <function doctree_read at 0x000002A150CA8D60> for event 'doctree-read' threw an exception (exception: 'charmap' codec can't decode byte 0x9d in position 23159: character maps to <undefined>)

Extension error (sphinx.ext.linkcode):
Handler <function doctree_read at 0x000002A150CA8D60> for event 'doctree-read' threw an exception (exception: 'charmap' codec can't decode byte 0x9d in position 23159: character maps to <undefined>)

This looks to me like an encoding issue. I don't currently have Windows set to UTF-8 mode, which is possible and works well but is not the default.

If instead I run Python in UTF-8 mode the build works. Maybe add set PYTHONUTF8=1 to the make.bat file? 🤔

sarahboyce
sarahboyce reviewed Jul 15, 2023
View reviewed changes
felixxm
felixxm reviewed Jul 17, 2023
View reviewed changes
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ewjoachim Thanks 👍 I left initial comments.

docs/_ext/github_links.py Outdated
import importlib.util
import pathlib

BASE_PATH = pathlib.Path(__file__).parents[2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we deffer accessing __file__ or don't use it here? (check out ticket-32316 and 9ee693b)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I had no idea. This seems like it should be part of the automated testing/linting steps rather than being manually checked, so that we avoid back and forth. Should we create a ticket for doing that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create a ticket for doing that?

This could be a part of ticket-30950. For now, we're trying to make life less painful for folks with frozen environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I got it. I'll try to do it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Also, this module will not be imported unless we're calling sphinx and while it could make sense to interact with the packaged doc (doc is present in the packaged tar.gz but not in the wheel), I'm not sure it's reasonable to expect this to work in a frozen env. So I'm pretty sure we were safe, but the change has been done nonetheless.

@smithdc1
Copy link
Member

I can't see [source] links for some items. Let's take django.forms.Form as an example. While in the 1.8 docs, there's a link. see docs. I can't see it with this build.

I wonder if that's due to the __init__.py file using from ... import *.

In the django.forms.Form case in get_path_and_line(), the locator.import_locations is {'ValidationError': 'django.core.exceptions', '*': 'django.forms.widgets'}.

It seems to me that the issue is that we're getting * rather than the actual items that would be imported by that? 🤔

What do you think?

@ewjoachim
Copy link
Contributor Author

Ha I had not foreseen that Django would have import * :D And all of them have NOQA which means we know it's not awesome but still do it...

I'm guessing we'll need to be smarter, then. I'll rework the code.

@ewjoachim
Copy link
Contributor Author

(Just for the record: as discussed on the Django Discord, I'm sorry that I wasn't able to take the time and take this contribution to the end, but I'd be happy to help anyone interested to start from there, including and especially Djangonauts from the Djangonaut Space program. Let me know here or even preferably on Discord (@ewjoachim).)

@felixxm
Copy link
Member

felixxm commented Nov 8, 2023

@ewjoachim Thanks for all your efforts 👍

Closing per Joachim's comment.

@felixxm felixxm closed this Nov 8, 2023
@smithdc1 smithdc1 mentioned this pull request Dec 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixxm felixxm felixxm left review comments

@sarahboyce sarahboyce sarahboyce approved these changes

+1 more reviewer

@smithdc1 smithdc1 smithdc1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DjangoCon Europe 🏰

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ewjoachim @smithdc1 @felixxm @sarahboyce