Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

chore: Switch from edx-sphinx-theme to sphinx-book-theme #426

Conversation

xitij2000
Copy link
Contributor

@xitij2000 xitij2000 commented Mar 24, 2023

The edx-sphinx theme is being deprecated, and replaced with sphinx-book-theme. This removes references to the deprecated theme and replaces them with the new standard theme for the platform.

Testing instructions:

  • Run make docs and see that the docs are generated properly and use the sphinx-book-theme

See openedx/public-engineering#200

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 24, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (4edda50) 86.13% compared to head (94144af) 86.13%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #426   +/-   ##
=======================================
  Coverage   86.13%   86.13%           
=======================================
  Files          11       11           
  Lines         952      952           
  Branches      156      156           
=======================================
  Hits          820      820           
  Misses         88       88           
  Partials       44       44           
Flag Coverage Δ
unittests 86.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@xitij2000 xitij2000 force-pushed the kshitij/switch-to-sphinx-book-theme branch from d719af8 to 39bca1a Compare March 24, 2023 07:42
@xitij2000 xitij2000 changed the title feat: Switch from edx-sphinx-theme to sphinx-book-theme chore: Switch from edx-sphinx-theme to sphinx-book-theme Mar 24, 2023
@mphilbrick211
Copy link

Hi @xitij2000 - looks like there's some failing tests on this one.

@xitij2000 xitij2000 force-pushed the kshitij/switch-to-sphinx-book-theme branch 2 times, most recently from 2bee43b to a0ad1d6 Compare March 28, 2023 06:02
@xitij2000
Copy link
Contributor Author

Hi @xitij2000 - looks like there's some failing tests on this one.

It seems the test failures are happening due to a change in linting rules which were updated during the requirements upgrade. So I'm limiting all these PRs to just changes to docs related requirements.

@xitij2000 xitij2000 force-pushed the kshitij/switch-to-sphinx-book-theme branch 2 times, most recently from 2e9aa0e to de6fbd1 Compare April 5, 2023 22:13
@xitij2000
Copy link
Contributor Author

The coverage failure seems to be due to reasons outside of this PR since this doesn't touch any non documentation code.
Given that this repo is to be archived anyway, would it be OK to merge this as-is, if at all?

@mphilbrick211
Copy link

The coverage failure seems to be due to reasons outside of this PR since this doesn't touch any non documentation code. Given that this repo is to be archived anyway, would it be OK to merge this as-is, if at all?

@jmbowman - flagging this for you. Thanks!

@Cup0fCoffee
Copy link

👍

  • I tested this: ran make docs
  • I read through the code

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

1 change, then I can merge.

docs/conf.py Outdated Show resolved Hide resolved
@xitij2000
Copy link
Contributor Author

@feanil Tests are failing in this and some other repos due to codecov being missing.

I can remove the dependency but am not sure if this is the appropriate PR for it. Perhaps as a separate commit in the PRs? Or do I wait till that is corrected in a separate PR before rebasing these PRs?

@feanil
Copy link
Contributor

feanil commented Apr 14, 2023

I've got a separate PR to fix it, that I'm waiting for review on: #431

@feanil
Copy link
Contributor

feanil commented Apr 18, 2023

@xitij2000 you should be ableto rebase this now and it should pass tests.

The edx-sphinx theme is being deprecated, and replaced with sphinx-book-theme.
This removes references to the deprecated theme and replaces them with the new
standard theme for the platform.

See https://github.com/openedx/edx-sphinx-theme/issues/184
@xitij2000 xitij2000 force-pushed the kshitij/switch-to-sphinx-book-theme branch from 20e9548 to 94144af Compare April 19, 2023 09:54
@xitij2000
Copy link
Contributor Author

@xitij2000 you should be ableto rebase this now and it should pass tests.

Done!

@feanil feanil merged commit 256ac95 into openedx-unsupported:master Apr 20, 2023
7 checks passed
@openedx-webhooks
Copy link

@xitij2000 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@xitij2000 xitij2000 deleted the kshitij/switch-to-sphinx-book-theme branch April 28, 2023 02:16
@itsjeyd itsjeyd added core contributor PR author is a Core Contributor (who may or may not have write access to this repo). and removed changes requested labels Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants