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

[INF/DOC] Documentation bugfix by pinning mkdocstrings to 0.18.1 #1147

Merged
merged 5 commits into from
Aug 15, 2022

Conversation

ericmjl
Copy link
Member

@ericmjl ericmjl commented Aug 14, 2022

@Zeroto521 and @samukweku please merge as soon as the docs are built correctly on the docs preview.

@ericmjl ericmjl changed the title [INF/DOC[ Documentation bugfix by pinning mkdocstrings to 0.18.1 [INF/DOC] Documentation bugfix by pinning mkdocstrings to 0.18.1 Aug 14, 2022
@ericmjl
Copy link
Member Author

ericmjl commented Aug 14, 2022

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #1147 (6cf4762) into dev (6c43b9d) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1147      +/-   ##
==========================================
- Coverage   97.34%   97.31%   -0.04%     
==========================================
  Files          77       77              
  Lines        3240     3240              
==========================================
- Hits         3154     3153       -1     
- Misses         86       87       +1     

@Zeroto521
Copy link
Member

Now documentation environment is mkdocs/environment.yaml.
I apply these you did to environment-dev.yml to mkdocs/environment.yaml

@ericmjl
Copy link
Member Author

ericmjl commented Aug 15, 2022

I see, @Zeroto521. Thanks for the push.

In working through this PR, I'm starting to see possible undesirable consequences of having separate environment definitions. Complexity is the main driver here.

Previously, we could all rely on a single source of truth for the environment definition -- environment-dev.yml. With a separate environment.yml for docs, I didn't think to look at it (mkdocs/environment.yaml), because (1) it wasn't stored at the top level (which makes sense given what you were trying to accomplish) and (2) the GitHub actions configurations were not fully updated (i.e. only docs-preview.yml was updated, but not docs.yml). I should have reviewed your PR more stringently, so I'm partially responsible for this mess I found myself in.

One of the principles we have in the pyjanitor project is to make contributing easy for newcomers; having different environment.yml files may lead to downstream confusion if newcomers don't know where to look. As an old-timer on the project, I also missed out on this point; maybe I need to update my knowledge.

Going forward, I think we should have a deeper discussion about having separate environments for docs and development. Just as we try to standardize development, testing, and deployment environments with a single dev container, having a single environment.yml follows the same spirit.

For now, I'm going to revert some of the changes that you made. This is just to fix broken docs, though, so I hope you don't feel that I'm rejecting the change outright. We can reintroduce them in a more comprehensive PR; if you could help me here by articulating (in PR #1133) what advantages we would get by your proposed changes, that will help stimulate the discussion further.

@ericmjl
Copy link
Member Author

ericmjl commented Aug 15, 2022

@Zeroto521 and @samukweku I added a very rudimentary automated check for the docs. We may want to see whether there are better options out there, though. I will tag you both in a comment on the relevant file.

@@ -0,0 +1,24 @@
"""Tests for documentation build."""
Copy link
Member Author

Choose a reason for hiding this comment

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

@Zeroto521 and @samukweku this is the automated hack I have in place to check that our docs are built correctly automatically... at least for the general functions page. We may want to do a more comprehensive thing later.

@ericmjl
Copy link
Member Author

ericmjl commented Aug 15, 2022

Finally, confirming that the docs built correctly on the deploy preview above:

image

@ericmjl
Copy link
Member Author

ericmjl commented Aug 15, 2022

Ok, I'm going to take an "executive" decision and merge, so that our friends in the Eastern Hemisphere timezones can access usable docs as their work week begins.

@ericmjl ericmjl merged commit fa6c284 into dev Aug 15, 2022
@Zeroto521 Zeroto521 deleted the bugfix/mkdocstrings-0.18.1 branch October 30, 2022 01:25
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