DOC: Clean up settings

[mgeier]

No description provided.

[mgeier]

Note: The "weather map" example is not included in the toctree, but it is available by URL: https://ipywidgets.readthedocs.io/en/latest/examples/Layout%20Example.html

It should probably be added to the toctree?

If this is not desired, it should be added to exclude_patterns?

[pbugnion]

I think I would vote against including the weather map since it brings in pandas and ipyleaflet. My feeling is that we already struggle to maintain the docs (people who spend more time than me doing this like Jason or Matt should chime in here if I'm wrong), so any additional complexity should be weighed very carefully. In this case, I think the extra burden of keeping this page working is not sufficient to justify keeping it in the docs.

We could migrate it to a stand-alone GitHub gist, for instance? I'd be happy to do that if that's useful. Alternatively, we can keep it and exclude it from the build.

The rest of the changes look good! I made a few comments inline.

[mwcraig]

I think I would vote against including the weather map since it brings in pandas and ipyleaflet.

I'm not too worried about the dependencies; each is a well-used package that should install without conflicts. It would be nice to add some more explanatory text to the example, but that is a separate issue.

[mgeier]

@pbugnion:

I think I would vote against including the weather map since it brings in pandas and ipyleaflet.

And this is a problem why?

[UPDATE: I saw @mwcraig's comment above (#2660 (comment)) after sending this one initially.]

My feeling is that we already struggle to maintain the docs (people who spend more time than me doing this like Jason or Matt should chime in here if I'm wrong), so any additional complexity should be weighed very carefully. In this case, I think the extra burden of keeping this page working is not sufficient to justify keeping it in the docs.

I'm not arguing against that, that's your (the project's maintainers) call.

But adding a notebook that's already in the repo to the rendered documentation IMHO doesn't add complexity.

Therefore, I don't see a reason to delay this PR.

If you (the maintainers) decide to remove the weather map notebook (or any other notebooks) from the repo, you can do that in a separate PR.

[mwcraig]

@pbugnion -- can I merge this? I see there are a flurry of PRs and don't want to generate merge conflicts unnecessarily...

[pbugnion]

I'm not too worried about the dependencies; each is a well-used package that should install without conflicts.

Sounds good, thanks for clarifying.

can I merge this?

Building the docs directly on this branch fails, because it's missing the fixes in this PR. The CI tests passed on this when it was first submitted because they ran with a now obsolete nbformat. Rebasing this PR off master works fine to build the docs.

I think I'd feel most comfortable with:

1. Rebasing off master (or merging master)
2. Letting the CI tests run
3. Merging

@mwcraig obviously happy to defer to you if you think that's overkill.

[mwcraig]

OK, @mgeier -- would you like to do the rebase or shall I?

[mwcraig]

@mgeier -- thanks for this, I'm going to rebase and then merge once tests pass.

[mwcraig]

Is there a way to change the Docs test so that it does not use Python 3.5? That goes EOL in several months and it seems like we should be using something newer. I'm not 100% sure yet that an old python version is the issue...

[pbugnion]

Is there a way to change the Docs test so that it does not use Python 3.5? That goes EOL in several months and it seems like we should be using something newer.

That sounds sensible. AFAIK, you just need to change the Python version here:

https://github.com/jupyter-widgets/ipywidgets/blob/master/.github/workflows/testdocs.yml#L13

Happy to do this tomorrow if that's useful.

[mwcraig]

I'll give it a try today and see if it helps, thanks

[mwcraig]

@mgeier -- any idea what might be causing this failure?

[mgeier]

I've never seen this error. I can probably take a closer look tomorrow.

[pbugnion]

@mwcraig we should probably use the same version of Python when testing building the docs and on readthedocs. I've switched both to 3.7 in PR #2684 .

[mwcraig]

@mgeier -- I'm going to take a quick look at ipyleaflet and see if I can track down the error.

@pbugnion -- agree that the testing versions should be the same.

[mgeier]

@mwcraig Seems like the method on_displayed() has been removed in #2021 by @jasongrout (merged by @SylvainCorlay).

I don't know how this can be fixed.

[mwcraig]

Good catch, and @jasongrout noted here that this would affect ipyleaflet.

What about allowing errors in this notebook for now?

I'm opening an ipyleaflet issue (jupyter-widgets/ipyleaflet#457) -- perhaps @martinRenou can fix it 😀 ?

[mwcraig]

In the interest of moving this forward, I'm going to:

• mark the map notebook as not-to-be-executed for now,
• open an issue in ipywidgets to remind us that it needs to be fixed (by ipyleaflet update and version restriction),
• and rebase, etc, to get the lights green on this.

The fact that we caught this here argues in favor fo including it to me -- we obviously can't test every downstream package, but some limited testing against ones in the jupyter-widgets org makes sense to me (we do something similar in astropy).

[mwcraig]

See #2663 for the issue reminding us to re-enable the notebook once ipyleaflet is updated.

[mwcraig]

@pbugnion -- if this looks ok to you, can you please merge? I noticed that #2684 has been closed without merging. This PR no longer changes to Python version in the tests, so that issue may be moot.

[mwcraig]

Also, thanks @mgeier for your fixes here!

[pbugnion]

Nice, thank you for pushing this through!

[pbugnion]

Thanks @mgeier !

[mgeier]

Thanks y'all!