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

Help wanted: Get documentation building with current Sphinx #2672

Open
swt2c opened this issue Jan 14, 2025 · 16 comments
Open

Help wanted: Get documentation building with current Sphinx #2672

swt2c opened this issue Jan 14, 2025 · 16 comments

Comments

@swt2c
Copy link
Collaborator

swt2c commented Jan 14, 2025

Currently, we are building the documentation with an old version of Sphinx, which resulting in us having to pin a bunch of Sphinx's dependencies and use an old Python.

Ideally, we'd get the documentation building with a current version of Python and Sphinx, fix the warnings/errors produced during the build, and be able to drop these pins.

See: https://github.com/wxWidgets/Phoenix/blob/master/requirements/devel.txt#L17

@neofelis2X
Copy link

Hi, I already tried to build the docs with recents sphinx not too long ago. I spent some more time on it now and have a branch that produces a pretty flawless result. I had to fix a lot of small things, but nothing dramatic.

You can take a look at my latest output of the docs here: https://wxpython-docs-test.netlify.app

More Details

I built the docs with: Python 3.12, Sphinx 8.1.3, on an arm64 Macbook

  • First I needed to pull PR Type stub improvements #2668 onto my branch because of Warnings when building documentation after type stubs changes #2666
  • Got Doxygen 1.9.1 because wxWidgets also uses that
  • jQuery was missing, therefore I needed to include sphinxcontrib.jquery
  • some updates to the sphinx conf.py where necessary
  • replaced getargspec with getfullargspec
  • had to repair sphinxtools/postprocessing.py because the html of sphinx 8 changed a bit
  • everything else are just small changes to the css or other bits

I appreciate any kind of feedback. If anyone finds errors please let me know. If you think this is useful I can push a PR.

@swt2c
Copy link
Collaborator Author

swt2c commented Jan 24, 2025

At first glance, it looks great! Thanks a lot for working on this.

@reticulatus any comments on these updates?

@reticulatus
Copy link
Contributor

@neofelis2X many thanks for this, it's a great improvement on the current state of the documentation!

I only have a few observations:

  1. In the pages that have a "Possible constructors" section (e.g. wx.font), each constructor has had "-> None" appended to it. I accept that when defining the class's __init__ method you don't explicitly return an instance of the class, but when the constructor is actually invoked at run time, an instance of the class is returned, so I feel the "-> None" statements are misleading here.

  2. In the pages that have a "Control Appearance" section (e.g. wx.button), the "wxGTK" labels don't line up with their images.

  3. Possibly out of scope for this issue, but there has been a long standing anomaly in the help page for the StyledTextCtrl (wx.stc.styledtextctrl). That page has a section titled "Index of the member groups" containing a list of links. In the corresponding wxWidgets page those links connect to other sections in the page that list the methods belonging to each group. However, in the wxPython case the links don't appear to go anywhere as the group sections are missing. It would be good if the group sections could be included. If that is not possible for some reason, then it would be better if the "Index of the member groups" section wasn't there as it serves no purpose in its current form.

@echoix
Copy link
Contributor

echoix commented Jan 25, 2025

I only have a few observations:

In the pages that have a "Possible constructors" section (e.g. wx.font), each constructor has had "-> None" appended to it. I accept that when defining the class's init method you don't explicitly return an instance of the class, but when the constructor is actually invoked at run time, an instance of the class is returned, so I feel the "-> None" statements are misleading here.

In Python, __init__ isn't a constructor, it initializes an object and shouldn't be returning anything, None is correct. __new__ (that there is no common reason for someone to use it), is closer to a constructor, would call __init__ and is will return the

https://codeql.github.com/codeql-query-help/python/py-explicit-return-in-init/

https://docs.python.org/3/reference/datamodel.html#object.__init__

https://docs.astral.sh/ruff/rules/return-in-init/

I don't think the goal is to create a C++ documentation, but a Python one, and (at least for newer sphinx) can use the type annotations to enhance the docs

@swt2c
Copy link
Collaborator Author

swt2c commented Jan 25, 2025

But on the other hand the "constructor" methods are also showing up as returning None which does not seem right, does it?

Image

@swt2c
Copy link
Collaborator Author

swt2c commented Jan 25, 2025

In any event, the constructor return type is probably more in scope for the work @lojack5 is doing with type hints.

@neofelis2X if the issues @reticulatus noted look easy to fix, that would be great, otherwise if you can make a PR with your changes, it would be much appreciated!

@lojack5
Copy link
Contributor

lojack5 commented Jan 25, 2025

Return types for __init__ if present should be None. It's sort of a style choice whether to type the returns for __init__s. I can make is so they're just untyped (more special case code! but it's all good)

@reticulatus
Copy link
Contributor

I'm not questioning return types for __init__

All I said was that return types for constructors should not be None.

>>> import wx
>>> f = wx.Font()
>>> f
<wx._core.Font object at 0x7226ea339be0>
>>> 

@lojack5
Copy link
Contributor

lojack5 commented Jan 25, 2025

That's the thing, in python __init__ is the closest concept to a constructor. There's __new__ as well, but that's almost never touched except in special cases. Constructors in C++ are converted to __init__'s in the python code.

@lojack5
Copy link
Contributor

lojack5 commented Jan 25, 2025

The fact that f is a wx.Font object in your example would be typed by the __new__ method, which the type-stubs aren't touching. Sphinx is grabbing the "possible constructors" from the same stuff that's generating the __init__s hence the -> None on them. Which brings me back to: since typing the return of __init__ is a style choice, I can just make it so they are untyped for the return.

@reticulatus
Copy link
Contributor

OK, I get what you are saying.

If that is the only way to prevent the "possible constructors" from showing "-> None", then personally I would prefer it, as it is less likely to confuse people, especially new users.

However, if others decide that it is more important that the __init__ methods include return type of None, so be it.

@lojack5
Copy link
Contributor

lojack5 commented Jan 25, 2025

Well without working on the sphinx generator itself (which I have no knowledge of) that's the quickest, easiest, and still technically correct way of doing it.

The issue stems from the whole idea of a constructor being split across two methods in python -> __new__ in general tells you the return type (some special classes won't return the same object type as the class they're in!), whereas __init__ generally has the arguments that are allowed for construction. Although that still isn't 100% correct because you can do some funky stuff in __new__ if you chose to. It's just that 99% of classes use object.__new__ as their __new__ implementation, so arguments for construction is in the __init__, but return type is in the __new__. (And aside-aside: technically all this can even be controlled even further, because what's really going on is the class's metaclass (usually type) has a __call__ method that is doing all of this...and you can override this behavior with your own metaclass...)

@swt2c
Copy link
Collaborator Author

swt2c commented Jan 25, 2025

Probably fine to leave it as-is.

@neofelis2X
Copy link

I only have a few observations:

...

If that is the only way to prevent the "possible constructors" from showing "-> None", then personally I would prefer it, as it is less likely to confuse people, especially new users.

  1. I will explore if I can easily hide the return value specifically for the Possible Constructors block. But it's probably not so easy.

  2. Good point! I fixed it. CalendarCtrl Example

  3. Wow, that's a lot of methods. (More than 760 to be specific) We are missing the categories here, which are usually added to the wxWidgets docs. I guess I could load them via an extra script and then apply them to the output. I will give it a try...

@Metallicow
Copy link
Contributor

and be able to drop these pins.

See: https://github.com/wxWidgets/Phoenix/blob/master/requirements/devel.txt#L17

Pins.... hmmmm.

@echoix
Copy link
Contributor

echoix commented Feb 3, 2025

and be able to drop these pins.

See: https://github.com/wxWidgets/Phoenix/blob/master/requirements/devel.txt#L17

Pins.... hmmmm.

Soon you'll be able to work normally, with a pyproject.toml in place.

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

No branches or pull requests

6 participants