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

Remove duplicates in documenting trait members #1

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

rkdarst
Copy link
Contributor

@rkdarst rkdarst commented Apr 15, 2020

  • When run with the :members:, then traitlets traits are duplicated,
    because they are added to the autodoc list from both from
    :members: and this autodetection. (I think)
  • Discussed in at least
    Jupyter(Hub) conceptual intro jupyterhub#2726. Currently
    causing JupyterHub docs to fail, because sphinx gives an error if
    there are duplicate autodoced traits and it is run with -W.
  • This seems to be started in a new version of sphinx, but we aren't
    completly sure. JH has been using the -W option since 2017.
  • I'm unsure if this is the right solution, but it works and gets me
    past these errors.

- When run with the `:members:`, then traitlets traits are duplicated,
  because they are added to the autodoc list from both from
  `:members:` and this autodetection. (I think)
- Discussed in at least
  jupyterhub/jupyterhub#2726.  Currently
  causing JupyterHub docs to fail, because sphinx gives an error if
  there are duplicate autodoced traits and it is run with `-W`.
- This seems to be started in a new version of sphinx, but we aren't
  completly sure.  JH has been using the `-W` option since 2017.
- I'm unsure if this is the right solution, but it works and gets me
  past these errors.
@rkdarst
Copy link
Contributor Author

rkdarst commented Apr 15, 2020

This needs proper testing. I edited live source files to test, then copied to here. Beware!

@betatim betatim requested a review from willingc April 16, 2020 06:00
@betatim
Copy link
Member

betatim commented Apr 16, 2020

If this fixes the problem in the JupyterHub docs build I would vote for merging this. My hunch is this is one of the packages that mostly only JupyterHub (or the JupyterHub ecosystem) uses. So if we break it more than it is already broken we will notice.

Let's see if Carol has time and thoughts.

@willingc
Copy link
Collaborator

This change seems reasonable to me.

I made some general comments in #3021.

  • The docs are building on RTD as of 2 weeks ago after Georgiana's change
  • If CI is failing due to the warnings, I would remove -W in CI.
  • Makes sense to me to convert to rst if @minrk agrees
  • I would also try to switch from autodoc to sphinx-autoapi.
  • Consider Sphinx 3 in the near future too ;-)

Copy link
Collaborator

@willingc willingc left a comment

Choose a reason for hiding this comment

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

LGTM. Though I did not build locally for this review.

@rkdarst
Copy link
Contributor Author

rkdarst commented Apr 16, 2020

I"ve been testing with a manual pip install https://... from JH, and it works. Last night, I thought that the sorting/ordering might need adjusting (if it's not sorted in the consumer), but... having the trait_members first actually sort of makes sense, so I guess it's not a big deal.

My concern about trivial syntax errors is resolved now, so if a minimal test is enough, I'd say this is OK to go.

@betatim betatim merged commit 75885ee into jupyterhub:master Apr 17, 2020
@betatim
Copy link
Member

betatim commented Apr 17, 2020

Merged. We are riding the elevator UPwards in the rabbit hole :)

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.

3 participants