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

Fixing Doc Build Warnings #1736

Merged
merged 17 commits into from
Jul 20, 2021
Merged

Conversation

isaacgsmith
Copy link
Member

@isaacgsmith isaacgsmith commented Jul 16, 2021

Description

This fixes all but 5 of the remaining documentation build warnings. The other 5 are fixed in #1721.

Most of the changes are very minor and had to do with formatting the documentation and docstrings.

The biggest changes that I should specifically mention are:

  • Deleting docs/scripts/index.rst. It is a duplicate of a section in docs/io/configuration/components/models/index.rst.
  • Fixing imports and commenting out some broken code in tardis/scripts/cmfgen2tardis.py. The entire document was already broken and is not being used in the code. I am just preventing errors from occurring when sphinx tries to run the file.
  • Commenting out the logotext keys in the html_theme_options dictionary in docs/conf.py as those keys are not valid.
  • Excluding ads.ipynb from nbsphinx. Upon merging, I will make a note of this in Improve list of papers using TARDIS #1599.

Motivation and context

After the merge of this PR, we will have an easy system to keep our documentation clean -- it will be easy to see if your PR gets sphinx to throw new warnings, and then you can fix them before merging.

How has this been tested?

  • Testing pipeline.
  • Docs built locally and on github.

Examples

Built documentation: https://smithis7.github.io/tardis/branch/warning_fix_doc/index.html

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Seems good, check the testing pipeline results as usual.

docs/conf.py Show resolved Hide resolved


atomic_dataset = AtomData.from_hdf5()
#atomic_dataset = AtomData.from_hdf()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this just commented?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't work, I left it as a comment for the same reason as the other one that I just replied to.

Copy link
Member

Choose a reason for hiding this comment

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

This script is broken into too many levels. Why modifying it right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@epassaro it created a warning

Copy link
Member

Choose a reason for hiding this comment

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

So this is a broken script. If someone tries to fix it in the future will ask: "why this line is commented?". And that brings confusion.

IMHO I don't think it's a good idea to patch a broken script just to silence a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment above the line to explain why it is broken. If we don't comment that part out, it messes up the API as well as defeating the purpose of getting rid of all warnings -- to make it easy to make sure future pull requests don't mess up the docs

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #1736 (6185207) into master (c6d8b91) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   61.87%   61.88%   +0.01%     
==========================================
  Files          63       63              
  Lines        5852     5851       -1     
==========================================
  Hits         3621     3621              
+ Misses       2231     2230       -1     
Impacted Files Coverage Δ
tardis/tardis/io/logger/logger.py 91.80% <0.00%> (ø)
tardis/tardis/io/atom_data/base.py 89.77% <0.00%> (ø)
tardis/tardis/montecarlo/spectrum.py 65.51% <0.00%> (ø)
tardis/tardis/scripts/cmfgen2tardis.py 0.00% <0.00%> (ø)
tardis/tardis/plasma/standard_plasmas.py 73.11% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6d8b91...6185207. Read the comment docs.

Copy link
Contributor

@DhruvSondhi DhruvSondhi left a comment

Choose a reason for hiding this comment

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

This looks good. Just a very small suggestion. Else everything looks good 🚀

docs/conf.py Show resolved Hide resolved
@DhruvSondhi DhruvSondhi self-requested a review July 18, 2021 05:07
Copy link
Contributor

@DhruvSondhi DhruvSondhi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please check & fix any other concerns.

@andrewfullard
Copy link
Contributor

One comment, I notice black is currently failing. Fix that and I'll merge.

@andrewfullard andrewfullard merged commit dfdbf04 into tardis-sn:master Jul 20, 2021
@isaacgsmith isaacgsmith deleted the warning_fix_doc branch July 20, 2021 22:31
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 1, 2021
* updating installation page

* adding debug config page

* fixing highlight warnings

* making images work

* fixing block formatting

* fixing docstring for api

* removing changelog update instructions

* fixing more warnings

* deleting scripts docs

* formatting index

* deleting section also in git_links.inc

* putting html_theme_options back in

* clearning warnings in cmfgen.py

* fixing docstrings for AtomData api

* including full file name to exclude ads notebook

* explaining why code was commented out

* formatting with black
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021
* updating installation page

* adding debug config page

* fixing highlight warnings

* making images work

* fixing block formatting

* fixing docstring for api

* removing changelog update instructions

* fixing more warnings

* deleting scripts docs

* formatting index

* deleting section also in git_links.inc

* putting html_theme_options back in

* clearning warnings in cmfgen.py

* fixing docstrings for AtomData api

* including full file name to exclude ads notebook

* explaining why code was commented out

* formatting with black
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021
* updating installation page

* adding debug config page

* fixing highlight warnings

* making images work

* fixing block formatting

* fixing docstring for api

* removing changelog update instructions

* fixing more warnings

* deleting scripts docs

* formatting index

* deleting section also in git_links.inc

* putting html_theme_options back in

* clearning warnings in cmfgen.py

* fixing docstrings for AtomData api

* including full file name to exclude ads notebook

* explaining why code was commented out

* formatting with black
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* updating installation page

* adding debug config page

* fixing highlight warnings

* making images work

* fixing block formatting

* fixing docstring for api

* removing changelog update instructions

* fixing more warnings

* deleting scripts docs

* formatting index

* deleting section also in git_links.inc

* putting html_theme_options back in

* clearning warnings in cmfgen.py

* fixing docstrings for AtomData api

* including full file name to exclude ads notebook

* explaining why code was commented out

* formatting with black
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants