Skip to content

Conversation

egpbos
Copy link
Member

@egpbos egpbos commented Mar 16, 2022

Removes building the docs from the build package workflow and adds a separate workflow for this. This way, failing workflow messages are immediately more informative.

@egpbos egpbos requested a review from sverhoeven March 16, 2022 14:44
Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

OK for me to have a separate job. Having it in its own workflow file follows the other workflows.

Instead of running the doc build step on a matrix of OSes and Python versions you now only use Ubuntu-latest + Python 3.9.
For the doctests it is helpful to test on the full matrix, as some could behave differently.
Not sure if I ever found bugs in doctests by running them on the full matrix. but this would be a feature that we would loose by merging this PR. I can live without it.

Why did you drop the matrix?

@egpbos
Copy link
Member Author

egpbos commented Mar 21, 2022

I dropped the matrix to get quicker and cleaner feedback; you don't have to wait in queue as long (quicker) and there is just one checkmark (cleaner). I think it's also pretty easy to add a matrix in for projects where it is needed, but like you I never ran into situations where a build matrix on documentation was necessary, so it is a less wasteful default option. In the end, you're going to have just one main documentation page (on readthedocs) anyway, so in my mind the doc test is mostly just there to see whether the docs compile (on readthedocs) or not.

@egpbos
Copy link
Member Author

egpbos commented Mar 30, 2022

Thoughts @sverhoeven?

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

Thanks for explaining.

I am also OK with running doc ci workflow for just the ubuntu/python39.

@egpbos egpbos merged commit 3c176c5 into NLeSC:main Mar 30, 2022
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.

2 participants