Skip to content

Fix sas.models suppression to not accidentally allow private members into sphinx docs#3954

Merged
llimeht merged 2 commits into
SasView:mainfrom
llimeht:tmp/doc-private-names
Jun 5, 2026
Merged

Fix sas.models suppression to not accidentally allow private members into sphinx docs#3954
llimeht merged 2 commits into
SasView:mainfrom
llimeht:tmp/doc-private-names

Conversation

@llimeht
Copy link
Copy Markdown
Contributor

@llimeht llimeht commented May 18, 2026

Description

Previous changes had allowed private names (variables, functions, methods starting with _ or __) to leak into the sphinx API docs. The result was that they were full of lots of meaningless items that should be hidden from the user. This patch restores the name (in)visibility in the API docs.

How Has This Been Tested?

The file that initially drew this to our attention was dev/sasview-api/sas.qtgui.Perspectives.ParticleEditor.datamodel.html - it came up because the private functions that were included into the docs had irreproducible signatures.

As seen in the CI outputs here, that file no longer contains these private names. This also makes the package reproducible, an important step in software assurance.

This patch also removes from very very old compatibility code for Sphinx 2.x.

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)
    • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

@butlerpd butlerpd requested a review from DrPaulSharp June 2, 2026 13:33
@butlerpd
Copy link
Copy Markdown
Member

butlerpd commented Jun 2, 2026

@pkienzle to review -- The immediate thought is that it is not a risky merge for 6.2 and it does affect the documentation built into sasview. Thus it is worth considering for that even though it is well past code freeze, though certainly not critical.

Copy link
Copy Markdown
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

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

Mostly ruff formatting changes.

As long as the docs build cleanly the impact will be minimal. The worst case is a broken link if a there is a references to a function or method that will be excluded.

I built the docs with this branch and verified that

  • a function with a leading underscore and a docstring in a module without __all__ defined does not appear in the documentation (sasmodels.product._tag_parameter in
    build/doc/dev/sasmodels-api/product.html), and that
  • a documented class whose method has a leading underscore and a docstring is also excluded (sasmodels.modelinfo.ParameterTable._get_defaults in build/doc/dev/sasmodels-api/modelinfo.html).
  • references of the form the regex :`[^`]*\b_ (there are a few) only exist in unincluded docstrings.

@llimeht
Copy link
Copy Markdown
Contributor Author

llimeht commented Jun 5, 2026

I think this is worth cherry-picking for 6.2.0 - the API documentation is close to unusable in its current form, as sphinx is picking up all manner of dunders that obscure the actually useful documentation. An example picked at random but is typical of the entire API docs currently in 6.2.0a3:

ViewerPlotDesign
    ViewerPlotDesign.__annotations__
    ViewerPlotDesign.__dataclass_fields__
    ViewerPlotDesign.__dataclass_params__
    ViewerPlotDesign.__dict__
    ViewerPlotDesign.__doc__
    ViewerPlotDesign.__eq__()
    ViewerPlotDesign.__firstlineno__
    ViewerPlotDesign.__hash__
    ViewerPlotDesign.__init__()
    ViewerPlotDesign.__match_args__
    ViewerPlotDesign.__module__
    ViewerPlotDesign.__replace__()
    ViewerPlotDesign.__repr__()
    ViewerPlotDesign.__static_attributes__
    ViewerPlotDesign.__weakref__
    ViewerPlotDesign.colour

There's a lot of noise in that, making the API docs pretty hard to read and navigate.

Up until sphinx 3.0, all of the names starting with _ would have been suppressed automatically. In dealing with breakage from sphinx 3.0 back in 2021, we (well, git blame tells me that I wrote it!) introduced the autodoc_skip_member_handler function to prevent sphinx from exploding when it saw the funny sas.models graft for sasmodels. That autodoc_skip_member_handler function has had a latent bug of only suppressing sas.models and not obeying the rest of the normal rules of sphinx. That hasn't been annoying enough until recently, with changes to Python (__annotations__), changes to sas internals (greater use of dataclass) and I suspect also changes to sphinx.

  • commit 4adf4bb is fixing that function and removes the now-legacy pre-sphinx 3.0 cruft (we are currently using sphinx 9.1). Cherry-pickable on its own if desired.
  • the second commit in the PR is running ruff on the surrounding code as encouraged by the ADR draft; that's obviously not needed for 6.2.0 but is also harmless to include at the same time.

For the _ items that @pkienzle identified, are these useful enough to someone using the package that they should be moved into the public API as future work?

@llimeht llimeht merged commit 74cb6d3 into SasView:main Jun 5, 2026
24 checks passed
@butlerpd
Copy link
Copy Markdown
Member

butlerpd commented Jun 5, 2026

Agreed! Exactly what I was thinking. This has been a longtime irritation so really appreciate your fixing it @llimeht

@DrPaulSharp
Copy link
Copy Markdown
Contributor

@jellybean2004 see the discussion here about potentially including this PR in the 6.2 release.

@pkienzle
Copy link
Copy Markdown
Contributor

pkienzle commented Jun 5, 2026

For the _ items that @pkienzle identified, are these useful enough to someone using the package that they should be moved into the public API as future work?

They are internal functions referenced in the docstrings for other internal functions. No need to document them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants