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

MAINT: use a function to test the warnings of the CI doc build #1002

Merged
merged 15 commits into from Oct 20, 2022

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Oct 10, 2022

FIX #937

new stuff:

  • A file listing all the warnings that we tolerate when the documentation is build. This list only show the warning after the keyword (WARNING or ERROR) to avoid issue with directory name (nox, not nox, hosted, virtualenv) and the file seperator between Linux and Windows
  • A test script that works in 3 steps:
    1. verify that all the expected warnings are here
    2. list the unexpected one
    3. if one of these list is not empty return 1 which stop the Action
  • some documentation in contribution on how to add new WARNING to the list

Let me know what you think

@12rambau 12rambau marked this pull request as ready for review October 10, 2022 13:40
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks nice, thanks - just a few quick comments. I'll also push a commit to clean up the language a bit, hope that's OK

tests/check_warnings.py Show resolved Hide resolved
WARNING: image file not readable: _static/gallery/feature-engine.png
WARNING: image file not readable: _static/gallery/arviz.png
WARNING: image file not readable: _static/gallery/sepal.png
WARNING: image file not readable: _static/gallery/enoslib.png
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this is going to get tedious because every new image here will create toil where we have to update these warnings. Could we allow this to support glob-like patterns and then do _static/gallery/*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the advantage here is that in 1 loop as I pop the element from the list I get both missing and unexpected. We already have a lot of examples in the gallery do you really expect it to increase that much?

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe not - let's not block this PR on that question, we can revisit if it becomes cumbersome or if people forget.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

A couple quick thoughts, this is looking pretty good to me though!

tests/check_warnings.py Outdated Show resolved Hide resolved
tests/warning_list.txt Show resolved Hide resolved
WARNING: image file not readable: _static/gallery/feature-engine.png
WARNING: image file not readable: _static/gallery/arviz.png
WARNING: image file not readable: _static/gallery/sepal.png
WARNING: image file not readable: _static/gallery/enoslib.png
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe not - let's not block this PR on that question, we can revisit if it becomes cumbersome or if people forget.

tests/check_warnings.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for this improvement to the maintainability of the theme!

@choldgraf choldgraf merged commit 299d974 into pydata:main Oct 20, 2022
@12rambau 12rambau deleted the warning branch October 20, 2022 09:02
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.

Better unexpected sphinx warnings check
2 participants