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 ReadTheDocs, update documentation #431

Closed
wants to merge 3 commits into from

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Mar 15, 2023

PR checklist

  • Short (1 sentence) summary of your PR:
    Fix ReadTheDocs build, update documentation
  • Developer(s):
    apcraig
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    no testing done, just doc changes
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

@apcraig
Copy link
Contributor Author

apcraig commented Mar 15, 2023

This PR is trying to debug the new readthedocs errors.

@apcraig
Copy link
Contributor Author

apcraig commented Mar 15, 2023

Killing this, need to debug another way.

@apcraig apcraig closed this Mar 15, 2023
@eclare108213
Copy link
Contributor

I wondered if it's actually a problem with the badge, since the documentation itself appeared to be fine. Is it not fine?

@apcraig
Copy link
Contributor Author

apcraig commented Mar 15, 2023

It's not the badge, that's the first thing I checked. I have created a new PR. Had to go thru the readthedocs logs carefully and do some googling. Seems to have been 2 minus signs that were "encoded" wrong. You can see the diff in the new PR, #432. But you can't tell there is a diff. I deleted the two minus signs and typed them back in. May have come from a copy and paste, hard to know.

If you look at the old documentation that had this, the html looks fine. The pdf was generated but these two minus signs were missing the pdf documentation.

Apparently, readthedocs changed behavior in the last day or two and it now fails the documentation if a pdf error occurs, wasn't doing that before.

The part I don't like is that I can create a PR to Icepack without the fixes and the github actions readthedocs check passes. So maybe that's just doing the html, not sure, but that could be a separate problem that we need to fix. I need to try to understand what github actions is doing and why it's not catching the pdf issue. May defer that, see how things go for a while.

@eclare108213
Copy link
Contributor

I'm not sure how many people actually look at the pdf - we certainly haven't checked it carefully. Should we remove pdf as an output option and just provide the html? Maybe this conversation should be moved to an issue.

@apcraig
Copy link
Contributor Author

apcraig commented Mar 15, 2023

I think the pdfs are nice to have. Certainly easier to have a pdf on your laptop offline than to try to download the html directory or not have access when offline. My feeling at this point is to see how this pans out. If the pdf causes us constant problems, then lets create an issue and discuss. In some ways, there was an error in the documentation (for latex anyway) and now we'll be trapping those more consistently, so that's good, I think.

Icepack docs badge is fixed with merge of #432. Will do the same for CICE next.

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