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

Use sphinx to create NVDA developer documentation #9968

Merged
merged 3 commits into from Jan 20, 2020

Conversation

@leonardder
Copy link
Collaborator

leonardder commented Jul 24, 2019

Link to issue number:

Fixes #9840

Summary of the issue:

When we switched to Python 3, we had to abandon Epydoc for developer documentation.

From the alternatives, sphinx seemed to be the most useful to us:

  1. It is the only supported api doc generator on Read the Docs
  2. It is used by numerous other projects, including python, wxPython, pyserial and configobj
  3. It is heavily extensible.
  4. In the end, it might allow us to integrate the user docs in the build process, thereby adding the ability to abandon txt2tags. Even if we don't, sphinx has markdown support with a simple extension.

Description of how this pull request fixes the issue:

This pr reimplements the functionality of scons devDocs, they are now build using sphinx.

Testing performed:

Tested that building the docs results into a structure of html files and that the build log doesn't contain any warnings related to parse errors of the source code.

Known issues with pull request:

  • The layout of the dev docs is likely to be broken, as we are still using epidoc style doc strings that have to be converted to the Python (restructured text) style doc strings
  • Read the Docs builds docs on linux. If we want to support this, it will require some source code changes. We can work around a lot of cases using mock imports. @derekriemer suggested that it might help if we build the docs ourselves on appveyor and push them afterwards. That's something that might be possible with RTD in the near future, but if I'm correct, it isn't possible yet.
  • No build in support for type hints, though there is an extension for that.

Incompatible doc string syntax

Currently, we use epytext/epydoc style annotations for our doc strings. such as:

@param test: a parameter
@type test: int

Sphinx uses directives like :param and :type. It also allows nesting them, e.g. :param test int: a parameter
Another important prerequisite is that sphinx doc strings require an empty line between the doc string body and the parameter info.

"""This is a doc string.
:param test: this fails and isn't parsed correctly (i.e. printer literally)

:param test: this succeeds and is marked up correctly.
"""

I've looked that sphinx-epytext but that doesn't handle the empty line case. It also just replaces @ with : in most cases.

I found an interesting tutorial that deals with the conversion using sed. I myself chose a different approach and wrote a prototype python conversion script

@feerrenrut suggested using 2to3 for this. This has to be done in a follow up pr, though.

Change log entry:

  • Changes for developers
    • Developer documentation is now build using sphinx. (#9840)
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Jul 25, 2019

I just looked at Sphinx AutoAPI which seems to offer a cleaner approach to build the docs by means of source code parsing. However, the logging of this tool is not very good. For example, if there is an error in a source file, the log file doesn't contain the name of the source file that actually failed. It also doesn't support surrogate characters in source files and seems to break on raw bytes of which it thinks they should UTF-8 continuation bytes.

@leonardder leonardder changed the base branch from threshold to master Jul 26, 2019
@leonardder leonardder force-pushed the leonardder:sphinx branch from b468ff8 to de61c30 Jul 26, 2019
@leonardder leonardder marked this pull request as ready for review Jul 29, 2019
@leonardder leonardder requested a review from feerrenrut Jul 29, 2019
@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Aug 13, 2019

hi leonardder
Thank you for your work
This pr is a good start, which is a good thing for translators and maintainers.
Perhaps in the near future, user guides and update logs can be maintained with poedit.

@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Oct 7, 2019

hi @michaelDCurran
What is the current progress of this pr, this pr is very important?
thank

@lukaszgo1

This comment has been minimized.

Copy link
Contributor

lukaszgo1 commented Oct 8, 2019

Can epydoc be removed from misc-deps while at it?

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 25, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit 915df16915

@@ -379,16 +380,16 @@ def _get_bookmark(self):
UIAHandler.UIA_TabItemControlTypeId,
UIAHandler.UIA_TextControlTypeId,
UIAHandler.UIA_SplitButtonControlTypeId
} if UIAHandler.isUIAAvailable else None

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 3, 2019

Contributor

This doesn't look like it should be in this change.

This comment has been minimized.

Copy link
@leonardder

leonardder Dec 3, 2019

Author Collaborator

You're correct, it makes sense to split this out.

@@ -1186,7 +1189,7 @@ def _get_keyboardShortcut(self):
UIAHandler.UIA_IsSelectionItemPatternAvailablePropertyId,
UIAHandler.UIA_IsEnabledPropertyId,
UIAHandler.UIA_IsOffscreenPropertyId,
} if UIAHandler.isUIAAvailable else set()

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 3, 2019

Contributor

Or this?

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Dec 3, 2019

There seem to be some unrelated changes in this PR. I think the next step is to get this building in the scons devDocs command. Does it cause errors if the doc strings are not converted? I think focusing on the minimal changes to get this building without errors first. Then look at converting the docstrings or finding an appropriate plugin.

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Dec 3, 2019

I'd prefer to see these pieces happen as separate PR's though, if possible.

@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Dec 3, 2019

I'd prefer to see these pieces happen as separate PR's though, if possible.

hi feerrenrut
We can split this pr, then review and merge.
thanks

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Dec 3, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit bdedb60376

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Dec 3, 2019

PR introduces Flake8 errors 😲

See test results for Failed build of commit ae97234d63

@leonardder leonardder force-pushed the leonardder:sphinx branch from 0e0e957 to 72de8ec Dec 3, 2019
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 3, 2019

I stripped this pr in such a way that it no longer contains changes to the NVDA source. All changes to NVDA's source that are required to make things work are no filed in separate pull requests: #10573, #10571 and #10574.

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Dec 4, 2019

Once #10573, #10571 and #10574 are merged, what is plan? I assume making sphinx docs build with the scons devDocs command. If that is happening without errors, then we can merge this in I think.

If you decide to go down the "roll your own" path for converting epydoc to sphinx you might want to consider using the 2to3 converter. It will handle iterating over the python files in a directory, and is smarter than regex about identifying python syntax. Modifying only doc strings should be much easier, if you hit that as a problem.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 5, 2019

I assume making sphinx docs build with the scons devDocs command.

Yes, that's True.

If you decide to go down the "roll your own" path for converting epydoc to sphinx you might want to consider using the 2to3 converter. It will handle iterating over the python files in a directory, and is smarter than regex about identifying python syntax. Modifying only doc strings should be much easier, if you hit that as a problem.

That's an interesting one. I'd say we should do that in a separate pr though.

@leonardder leonardder force-pushed the leonardder:sphinx branch from 72de8ec to 41431f4 Dec 6, 2019
@dingpengyu

This comment has been minimized.

Copy link
Contributor

dingpengyu commented Dec 7, 2019

hi @leonardder I checked the files about the development documents uploaded by pr 19430 and found the following problems:

  1. The uploaded file is not compressed, it is recommended to upload it to appveyor after compression
  2. I hope that the development documents can have a format file of gettext.pot so that translators can localize the developer documents.
    thanks
@leonardder leonardder force-pushed the leonardder:sphinx branch from b1d4dd4 to cf68db2 Dec 10, 2019
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 10, 2019

hi @leonardder I checked the files about the development documents uploaded by pr 19430 and found the following problems:

1. The uploaded file is not compressed, it is recommended to upload it to appveyor after compression

2. I hope that the development documents can have a format file of gettext.pot so that translators can localize the developer documents.
   thanks

Publishing devDocs in the output folder was a test to see whether the devDocs build on appveyor. I disabled this again as we never did this before, however I'm intending to revisit this at some point in a follow up.

@leonardder leonardder requested a review from feerrenrut Dec 10, 2019
Copy link
Contributor

feerrenrut left a comment

In appveyor yml we refer to "developerGuide" when doing a release. Having moved the developerGuide file and the release path being harder to test, there is potential for a change in behavior here. What do you think?

Could you update the testing to talk about the testing you have done with Appveyor?

devDocs/sconscript Outdated Show resolved Hide resolved
devDocs/devDocsInstall/.gitignore Outdated Show resolved Hide resolved
devDocs/.gitignore Outdated Show resolved Hide resolved
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 13, 2019

I think that we should publish the changes, user guide and developer guide with every build, which makes it easier to inspect them.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 13, 2019

Could you update the testing to talk about the testing you have done with Appveyor?

I have temporarily added the devDocs target to the appveyor scons targets and the build succeeded. Note that I only did this to see whether the dev docs would be able to build on appveyor. I don't think we should build them by default just yet.

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Jan 14, 2020

This looks ok, is there more work remaining before we merge this?

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Jan 14, 2020

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Jan 20, 2020

I have built this locally, bear in mind there are many warnings and a few errors. These will have to be fixed piece by piece, a single large PR will be hard to manage.
The following showed up a few times for the vision framework: TypeError: 'vision' object does not support item assignment

Full error:

WARNING: autodoc: failed to import module 'screenCurtain' from module 'visionEnhancementProviders'; the following exception was raised:
Traceback (most recent call last):
  File "C:\Users\Reef\AppData\Local\Programs\Python\Python37-32\lib\site-packages\sphinx\ext\autodoc\importer.py", line 32, in import_module
    return importlib.import_module(modname)
  File "C:\Users\Reef\AppData\Local\Programs\Python\Python37-32\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "C:\work\repo\nvda\6\source\visionEnhancementProviders\screenCurtain.py", line 292, in <module>
    class ScreenCurtainProvider(providerBase.VisionEnhancementProvider):
  File "C:\work\repo\nvda\6\source\visionEnhancementProviders\screenCurtain.py", line 293, in ScreenCurtainProvider
    _settings = ScreenCurtainSettings()
  File "C:\work\repo\nvda\6\source\vision\providerBase.py", line 42, in __init__
    self.initSettings()  # ensure that settings are loaded at construction time.
  File "C:\work\repo\nvda\6\source\autoSettingsUtils\autoSettings.py", line 107, in initSettings
    self._initSpecificSettings(self, self.supportedSettings)
  File "C:\work\repo\nvda\6\source\autoSettingsUtils\autoSettings.py", line 89, in _initSpecificSettings
    config.conf[section][settingsId] = {}
TypeError: 'vision' object does not support item assignment
@feerrenrut feerrenrut merged commit e1e4a55 into nvaccess:master Jan 20, 2020
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jan 20, 2020
@feerrenrut feerrenrut modified the milestones: 2019.3, 2020.1 Jan 20, 2020
feerrenrut added a commit that referenced this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.