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

Make the sphinx extension testable and more modular #1132

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jan 31, 2025

This PR is opened as a draft to share my work early, as I make progress on making it the sphinx extension into a suite of well-tested directives.

So far, it does seem necessary to produce a full sphinx run to test things properly, and for now I'm reusing our application's sphinx config.
We could define narrower testing configs, but I'm not sure that's a worthwhile endeavor, since our goal is to make sure we're testing our extension code here.

Type checking is also being addressed module-by-module as I move components around.
We should end up with fully tested and annotated sphinx extension code.


📚 Documentation preview 📚: https://globus-sdk-python--1132.org.readthedocs.build/en/1132/

@sirosen sirosen added the no-news-is-good-news This change does not require a news file label Jan 31, 2025
`globus_sdk._sphinxext` will now be a subpackage dir with all of the
modules it needs self-contained.
- Add a couple of unit tests guarded with importorskip (in a nice
  fixture)
- Add a relevant tox factor for 'sphinxext' testing
- Use that factor in the env_list and CI
The new sphinxext layout, including testability, allows us to better
pursue annotation and testing coverage of the sphinx extension code.
As such, it is de-listed from coverage and mypy exemptions, and needs
to start getting updates, including some minor fixes here.
And update mypy usage to ensure that type checking passes on the newly
isolated class.

In particular, update typing dependencies to include `sphinx`.
1. Move the directive to a new module, alongside the base
   AddContentDirective class
2. Add dedicated unit tests for it -- this required expansion of the
   sphinx extension testing toolchain to cover a full `sphinx`
   invocation (in-process)

I arrived at (2) after trying to integrate the docutils runner I had
built with sphinx directly. Sphinx has too many customizations and
patches -- the result was imperfect and fragile imitation of sphinx's
own internal behaviors.
New unit tests exercise listknownscopes.
@sirosen sirosen force-pushed the testable-sphinxext branch from 2b94681 to de0bbad Compare January 31, 2025 21:34
Move this sphinx directive to a dedicated module and test it from
there.
Move these components to dedicated sphinx-extension-specific modules
and add unit tests for them.
Move this sphinx directive to a new dedicated module and add some
basic unit tests for it.
Move autodoc signature hook to a dedicated module and add unit tests
for it.
Move to a dedicated module and add unit tests.
Move the CopyParams directive into the custom directives subpackage,
and add tests for it.

Additionally, move the param reader utility into the sphinx plugin
utility functions, and relocate its (only) unit test.
1. Move `custom_directives` to `directives` to match `roles`
2. Ensure the tests can run on py3.8 -- avoid contextmanager syntax
   which makes test collection fail
3. Fixup tox.ini to list `-sphinxext` tests in depends
4. Add a hint to the sphinx setup func for mypy
The Windows and macOS builds were failing coverage_report because they
were not running the new suite of tests. Additionally, covering these
specialized environments ensures a good, uniform development
experience.
@sirosen sirosen marked this pull request as ready for review February 1, 2025 03:28
Requiring an old version of `cryptography` forces an sdist build on
the macOS runner, which fails due to OpenSSL version mismatches.
When reading and writing files, don't allow for platform/locale
selection of encodings -- explicitly use UTF-8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant