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

Re-export more names in sphinx.domains.python #12297

Merged
merged 11 commits into from Apr 17, 2024

Conversation

JacobChesslo
Copy link
Contributor

Subject: Re-export various names in sphinx.domains.python

Feature or Bugfix

  • Bugfix

Purpose

  • Re-export various names in sphinx.domains.python

Detail

  • Re-export various names in sphinx.domains.python

Relates

@JacobChesslo JacobChesslo changed the title Re-export various names in sphinx.domains.python Draft: Re-export various names in sphinx.domains.python Apr 17, 2024
@JacobChesslo JacobChesslo changed the title Draft: Re-export various names in sphinx.domains.python Re-export various names in sphinx.domains.python Apr 17, 2024
@JacobChesslo
Copy link
Contributor Author

I think given the initial blast radius of #12295, it may be worth making these private methods more accessible and/or putting them on a more rigorous deprecation schedule.

@chrisjsewell
Copy link
Member

it may be worth making these private methods more accessible and/or putting them on a more rigorous deprecation schedule.

hard -1, if you are using private methods, then that is entirely your own risk; there is enough public API to worry about already 😅

@jbms
Copy link
Contributor

jbms commented Apr 17, 2024

sphinx-immaterial heavily monkey patches sphinx to add functionality and indeed it is on us to deal with changes in sphinx.
That said, monkey patching sphinx is not exactly unusual --- and sphinx itself does some monkey patching of docutils. We would like to integrate this functionality upstream but there are challenges with that. Exactly what name they have or whether they start with an underscore doesn't really matter but renames do create additional work for us.

@chrisjsewell
Copy link
Member

sphinx itself does some monkey patching of docutil

oh trust me, I'm not happy about that either 😄

monkey patching sphinx is not exactly unusual
Exactly what name they have or whether they start with an underscore doesn't really matter but renames do create additional work for us.

but yes, whether it usual or not (I wouldn't say so much), its absolutely not within the "contract" of any package to care or account for this type of usage.
If you do, especially without the knowledge of the upstream package, then you are just creating tech debt for yourself

We would like to integrate this functionality upstream but there are challenges with that.

This is the way 😁

# Conflicts:
#	sphinx/domains/c/__init__.py
#	sphinx/domains/cpp/__init__.py
@AA-Turner AA-Turner changed the title Re-export various names in sphinx.domains.python Re-export more names in sphinx.domains.python Apr 17, 2024
@AA-Turner AA-Turner merged commit be2b083 into sphinx-doc:master Apr 17, 2024
23 checks passed
@2bndy5
Copy link

2bndy5 commented Apr 17, 2024

My main concern is the refactor of public API members (PyTypedField) into private modules (sphinx.domains.python._object). While these re-exports deal with backward compatibility, they might be considered "unused imports" at some point in the future, thus a possible subject for removal in 2 major versions.

Many linters have warnings about importing from private modules, so any third-party extension that might want to implement their own autodoc behavior would have to either

  • hope what they need has been re-exported via public modules
  • ignore the linter warning and import from private modules

Simply put, it would be more PEP8 compliant if the any refactored modules that contain previously public API to be renamed as public modules. Change sphinx.domains.python._object -> sphinx.domains.python.object.

@chrisjsewell
Copy link
Member

Simply put, it would be more PEP8 compliant if the any refactored modules that contain previously public API to be renamed as public modules.

Not that's not necessary, you just use __all__, as discussed here: #12246

@2bndy5
Copy link

2bndy5 commented Apr 17, 2024

Using __all__ is certainly a solution, but maintaining that is additional tech debt. That said, it might be a good solution if one of the new private modules contains both public and private API.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants