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 docstrings & indentation related warnings for Sphinx #1617

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

DhruvSondhi
Copy link
Contributor

@DhruvSondhi DhruvSondhi commented Jun 4, 2021

This PR aims to fix all the warnings related to the docstring & indentations that are being generated when building the documentation.
Docstrings have been updated successfully.
Most of the indentation errors have been fixed.

Documentation Preview: https://dhruvsondhi.github.io/tardis/branch/fix_docstring_errors/index.html

Description

Some of the docstring had unexpected indentations & hence rose warnings whenever building the documentation
A notable example can be seen here:
before --> Check here
after --> Check here

Some of the documentation pages had wrong directives for code blocks. Those have been taken care of in the Pull Request.
This removes these warnings & hence makes it easier to build the documentation

Motivation and context

Fixing Sphinx Warning will allow for better generation of the Documentation via Sphinx.

How has this been tested?

  • Testing pipeline.
  • Other. Created the documentation on my fork

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.

@github-actions
Copy link

github-actions bot commented Jun 4, 2021

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

  • Is the necessary information provided?
    • Does the PR have a complete description? Does it explain what the PR is attempting to do/fix, does it explain how it does this?
    • Is there an explanation of why this PR is needed?
    • Please use the TARDIS PR template
  • 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 at or very close to 100%.
  • Is the code properly documented?
    • If this modifies existing code, then the docs should be updated. If this adds a new feature, additional documentation should be created.
    • Sphinx and docstrings in the code (in numpydoc format)
  • Does this conform to PEP 8 and the TARDIS style guidelines?
  • Does the PR fix the problem it describes?
    • Make sure it doesn’t e.g. just fix the problem for a specific case
    • Is this the best way of fixing the problem?
  • Is the code tidy?
    • No unnecessary print lines or code comments

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #1617 (2ee1996) into master (e062c69) will not change coverage.
The diff coverage is n/a.

❗ Current head 2ee1996 differs from pull request most recent head aa59a8c. Consider uploading reports for the commit aa59a8c to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1617   +/-   ##
=======================================
  Coverage   67.20%   67.20%           
=======================================
  Files          73       73           
  Lines        6150     6150           
=======================================
  Hits         4133     4133           
  Misses       2017     2017           
Impacted Files Coverage Δ
tardis/tardis/io/atom_data/base.py 90.80% <0.00%> (ø)
tardis/tardis/montecarlo/packet_source.py 97.29% <0.00%> (ø)
tardis/tardis/plasma/properties/atomic.py 61.76% <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 802fab9...aa59a8c. Read the comment docs.

@DhruvSondhi DhruvSondhi force-pushed the fix_docstring_errors branch 2 times, most recently from 4aad62c to ec61e19 Compare June 4, 2021 08:26
@@ -98,8 +98,8 @@ class Simulation(PlasmaStateStorerMixin, HDFWriterMixin):
nthreads : int
The number of threads to run montecarlo with

.. note:: TARDIS must be built with OpenMP support in order for
`nthreads` to have effect.
.. note:: TARDIS must be built with OpenMP support in order for ``nthreads`` to have effect.
Copy link
Member

Choose a reason for hiding this comment

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

How should we rewrite this now that we do not use OpenMP @andrewfullard

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this pull request. However, it should be changed. The note for that variable should be removed, assuming it is still used in the class.

@DhruvSondhi DhruvSondhi force-pushed the fix_docstring_errors branch from 55d6c5f to f45a518 Compare June 4, 2021 15:23
@tardis-sn tardis-sn deleted a comment from github-actions bot Jun 4, 2021
@tardis-sn tardis-sn deleted a comment from github-actions bot Jun 4, 2021
@DhruvSondhi DhruvSondhi force-pushed the fix_docstring_errors branch from f45a518 to 2fd375e Compare June 5, 2021 09:26
@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.

@tardis-sn tardis-sn deleted a comment from tardis-bot Jun 8, 2021
@DhruvSondhi DhruvSondhi force-pushed the fix_docstring_errors branch from 2fd375e to aa59a8c Compare June 9, 2021 07:24
@andrewfullard andrewfullard merged commit 8ceacc0 into tardis-sn:master Jun 9, 2021
@DhruvSondhi DhruvSondhi deleted the fix_docstring_errors branch June 9, 2021 18:25
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
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.

4 participants