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

Fix documentation build issues #321

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Fix documentation build issues #321

merged 1 commit into from
Jun 4, 2024

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jun 4, 2024

Our documentation build on read-the-docs (RTD) is failing. This should fix it, hopefully.

Because this can only be fully tested by merging the PR and monitoring the doc build in RTD, I will merge this soon.

Ideally, we get rid of RTD for the following reasons:

  • RTD supports only one admin account per project, i.e. bus-factor 1 by construction, unless one shares passwords, which is a bad idea anyway.
  • Testing the RTD build is only possible after merging.

As an alternative, we can build the documentation in a GitHub Action and host it on gh-pages instead.

Copy link

deepsource-io bot commented Jun 4, 2024

Here's the code health analysis summary for commits 6435bc3..034164e. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource Python LogoPython✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tovrstra - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -102,7 +102,7 @@
#
# This is also used if you do content translation via gettext catalogs.
# Usually you set "language" from the command line for these cases.
language = None
language = "en"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Hardcoding language to 'en' might limit flexibility.

Consider making the language configurable via an environment variable or a command-line argument to allow for easier localization in the future.

Suggested change
language = "en"
import os
language = os.getenv('DOC_LANGUAGE', 'en')

@@ -0,0 +1,28 @@
# Read the Docs configuration file for Sphinx projects
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider enabling 'fail_on_warning' for stricter builds.

Enabling 'fail_on_warning' can help catch issues early by failing the build on any warnings, ensuring higher quality documentation.

Suggested change
# Read the Docs configuration file for Sphinx projects
# Read the Docs configuration file for Sphinx projects
# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details
fail_on_warning: true

Copy link
Member

@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

I agree, let's ditch read-the-docs. We're mostly using GitHub pages for everything else now anyway, and it's much nicer to deal with.

@tovrstra tovrstra merged commit 35fd980 into theochem:main Jun 4, 2024
10 checks passed
@tovrstra tovrstra deleted the fix-rtd branch June 4, 2024 17:54
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