Skip to content

Conversation

@olivialynn
Copy link
Member

@olivialynn olivialynn commented Mar 22, 2023

Some notes:

  • Added sphinx building to the testing-and-coverage workflow. Not against taking this out of the unit test workflow and creating a doc-building workflow, if people prefer that.
  • Made a number of changes to the Makefile. I'm not terribly up to date on best practices wrt those, so this is an area where I'm especially open to hear feedback.
  • This PR didn't end up troubleshooting the jupyter-nb-clear-output pre-commit hook after all (but it solves the sphinx pre-commit check failing with notebooks, though). I had trouble getting its included command to even work from the command line, and was already way past the time I'd budgeted to this. It would definitely be great to resolve that at some point.

@olivialynn olivialynn marked this pull request as ready for review March 22, 2023 13:37

.PHONY: help Makefile
# Build all Sphinx docs locally, except the notebooks
no-nb no-notebooks:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really helpful!

@$(SPHINXBUILD) -M html "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(EXCLUDENB) $(O)

# Cleans up files generated by the build process
clean:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

SOURCEDIR = .
BUILDDIR = ../_readthedocs/

.PHONY: help no-nb no-notebooks clean Makefile
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am used to seeing .PHONY at the end of the Makefile but I do not know if it matters. Its sole purpose is to declare targets that do not represent actual files. So I do not think that Makefile needs to be present (unless you noticed some error?)

Copy link
Member Author

@olivialynn olivialynn Mar 22, 2023

Choose a reason for hiding this comment

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

The initial Makefile that Sphinx generates has the line .PHONY: help Makefile, so I reasoned it was best keep it if I had no specific reason to remove it.

Based on this SO, it seemed to me like it's there to make sure whatever targets are captured by %: Makefile are always run, and--this part goes a little over my head--the presence of Makefile as a dependency of % to avoid weird GNU Make behavior. (But please let me know if you have any thoughts!)

@drewoldag
Copy link
Collaborator

This looks great @olivialynn, nice work!

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.

5 participants