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

Move to mkdocs from Sphinx #135

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Move to mkdocs from Sphinx #135

merged 2 commits into from
Jul 15, 2024

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Jun 7, 2024

There are a lot of changes required for this PR, most of them minor:

  1. Update RST syntax to Markdown (e.g. URLs, anchors to clases/func, code blocks, lists)
  2. Make the linter happy (lines too long with new URLs)
  3. Consolidate tests (see Package code and tests should be organized in separate directories, to be more consistent with Python conventions #120)
  4. Fixed a few other docstring warnings

@nh13 nh13 requested a review from tfenne June 11, 2024 23:00
@nh13 nh13 marked this pull request as ready for review June 11, 2024 23:00
@nh13
Copy link
Member Author

nh13 commented Jun 11, 2024

I would love a review of the live preview. I will admit I am a mkdocs neophyte, so I would welcome pull requests into this branch if you want to noodle on any non-trivial requested changes.

@nh13 nh13 changed the title WIP: mkdocs Move to mkdocs from Sphinx Jun 11, 2024
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.38%. Comparing base (04c1a9c) to head (6f4e685).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage   88.38%   88.38%           
=======================================
  Files          16       16           
  Lines        1773     1773           
  Branches      377      377           
=======================================
  Hits         1567     1567           
  Misses        136      136           
  Partials       70       70           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@znorgaard
Copy link
Contributor

I took a quick look at the live preview. Overall, I like the layout and functionality!

Dark Mode

This could be a future enhancement, but a dark mode toggle and defaulting to "auto" to match system settings would be great: https://www.mkdocs.org/user-guide/choosing-your-theme/.

Leftover Styling

I think there's some leftover styling that displays oddly.

There are a handful of ":class:ClassName" instances, example below, but search will find them all.

https://fgpyo--135.org.readthedocs.build/en/135/reference/fgpyo/index.html#fgpyo.fastx.FastxZipped-functions
image

":py:obj:" seems like a styling leftover too.

https://fgpyo--135.org.readthedocs.build/en/135/reference/fgpyo/index.html#fgpyo.fastx--zipping-fastx-files
image

Installation Page

Looks like some differences in how to reference page titles.

https://fgpyo--135.org.readthedocs.build/en/135/installation.html
image

@tfenne
Copy link
Member

tfenne commented Jun 12, 2024

After poking for a few minutes at the live preview - I like it a lot! I really like:

  • That modules are broken into separate pages so you can easily CMD-S and search the text of a module page, and not accidentally scroll into other modules
  • I really like the module index that shows up on the right when you click into a module.

Big usability improvement from my perspective.

Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

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

I made a quick pass through. Overall I think this is an improvement, but I have several thoughts:

  1. Personally I really like the Markdown over RST - I think it's more readable likely less fragile.
  2. I think we can and should strike all the "Module Contents" sections from the docs. The right-nav on the module page does a good job of this automatically and we've always struggled to keep these sections up to date as new things get added.
  3. I really wish there was a better mechanism for linking to other python symbols vs. including a relative markdown link.
  4. The script for auto-discovering modules etc. - I would love for this to be something that is parameterized, and better documented, and then sharable either from fgpyo or as a standalone repo that can be sub-moduled in or something, rather than copy/pasting it between repos.

docs/scripts/gen_ref_pages.py Show resolved Hide resolved
fgpyo/collections/__init__.py Outdated Show resolved Hide resolved
fgpyo/collections/__init__.py Outdated Show resolved Hide resolved
@msto msto mentioned this pull request Jun 17, 2024
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:34 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:34 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:37 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:37 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:37 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:37 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:37 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:40 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:40 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:40 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:40 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci June 26, 2024 07:40 — with GitHub Actions Inactive
Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

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

I think this is a big step forward on usability of the docs, and maintainability too!

docs: update docstrings for proper indentation

tests: move tests to their own directory
docs/scripts/gen_ref_pages.py Outdated Show resolved Hide resolved
@nh13 nh13 temporarily deployed to github-actions-ci July 13, 2024 21:00 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 13, 2024 21:00 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 13, 2024 21:00 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 13, 2024 21:00 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 13, 2024 21:00 — with GitHub Actions Inactive
@nh13
Copy link
Member Author

nh13 commented Jul 15, 2024

My plan for making the gen_ref_pages.py applicable to all our repos is as follows:

  1. Add support for passing arguments to the gen_ref_pages.py in the mkdocs-gen-files repo (PR submitted). We'll need it merged and pushed into a release.
  2. Move the gen_ref_pages.py to it's own fulcrumgenomics GitHub repo and start adapting it there.
  3. Once we're happy with (2), release it to pypi
  4. Once (3) is complete, add (3) as a docs dependency in fgpyo

This may take a little while, but in the meantime I will merge this PR.

@nh13 nh13 merged commit fbe5923 into main Jul 15, 2024
8 checks passed
@nh13 nh13 deleted the docs/mkdocs branch July 15, 2024 04:35
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.

3 participants