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

Cleanup rst files #179

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Cleanup rst files #179

merged 1 commit into from
Apr 4, 2023

Conversation

PicoCentauri
Copy link
Collaborator

@PicoCentauri PicoCentauri commented Mar 30, 2023

  • Add linter for sphinx rst files
  • Remove trailing spaces
  • Convert README.md to README.rst (Preparations for later code sharing between REDME and docs)
  • Use 88 chars/line for rst files

📚 Documentation preview 📚: https://scikit-matter--179.org.readthedocs.build/en/179/

@PicoCentauri PicoCentauri marked this pull request as ready for review March 30, 2023 21:55

+---------------------------+------------+
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if the indentation is correct here. It was inconsistent between the dataset files. If I read the docs correctly there should be no indentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems, to look correct. Cannot judge what is correct according to sphinx. Shouldn't a warning be raised if it is not correct? I could not see any in the build on readthedocs

Copy link
Collaborator

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Thanks for the work. Just need some clarifications before merge: Did you use some rst file formatter for this? We probably want to include such a formatter in the tox format testenv for developers. As far as I understand black ignores rst files if you do not explicitly mention same. I don't have any experience with this, only found this https://github.com/LilSpazJoekp/docstrfmt

Convert README.md to README.rst (Preparations for later code sharing between REDME and docs)

Can you explain this a bit more? I am not sure what the reason is to change to rst.

@PicoCentauri
Copy link
Collaborator Author

Thanks for the review. You raised the right questions!

Thanks for the work. Just need some clarifications before merge: Did you use some rst file formatter for this?

So far not. I just used the rewrap plugin for vscode to do all the nasty formatting

We probably want to include such a formatter in the tox format testenv for developers.

Yes, we should add something. I already tooked around but I was not able to find the perfect plugin. We should include /flake8-docstrings. However, this will raise a lot of errors because it once the docstring in a very special format i.e docstrings for all public files, functions even in tests etc. We can disable these though.

As far as I understand black ignores rst files if you do not explicitly mention same. I don't have any experience with this, only found this https://github.com/LilSpazJoekp/docstrfmt

For rst files there are several linters and formatters but all of them have problems with the sphinx specific sections. I checked

Your looks quite promising though! I will have a look.

Convert README.md to README.rst (Preparations for later code sharing between REDME and docs)

Can you explain this a bit more? I am not sure what the reason is to change to rst.

Yes sure!. There are a lot of duplications in the README and the documentation pages on the one hand and on the other things which are in the README are missing in the documentation. If we have an rst file we can create markers and partly include text from the README in the documentation. This very useful but only works if the README is an rst file.

@PicoCentauri
Copy link
Collaborator Author

I checked docstrfmt. It is quite asgressive but looks good. The only thing is that it fails due to our citations I get an ValueError

ValueError: Unknown node type citation_reference!

I will investigate this, but maybe we do this after this PR?

@PicoCentauri
Copy link
Collaborator Author

Sphinx-lint seems to have a line width option. If this works this is enough for us I would say and we do not need an additional formatter. However, I tried it and it seems not to work... 😅

@PicoCentauri PicoCentauri force-pushed the docs_cleanup branch 2 times, most recently from 98a7ad2 to b0abe6c Compare April 3, 2023 12:18
@PicoCentauri
Copy link
Collaborator Author

I fixed the options of the rst linter. The length of each line is now also checked.

- Add linter for sphinx rst files
- Remove trailing spaces
- Convert README.md to README.rst (Preparations for later
  code sharing between REDME and docs)
- Use 88 chars/line for rst files
@agoscinski
Copy link
Collaborator

agoscinski commented Apr 4, 2023

but sphinx-lint is only checking and not formatting. My suggestion, we use for formatting docstrfmt and checking, it is only for the files bibliography.rst and selection.rst. We can ignore the formatting of the bibliography.rstand change the citation references in selection.rstto links. What do you think?

EDIT:
And we open an issue on rstfmt to fix this https://github.com/dzhu/rstfmt

@PicoCentauri
Copy link
Collaborator Author

Yes, we can definitely go for docstrfmt. But, should we first merge this and do it an extra PR? We already have a lot of changes already.

Copy link
Collaborator

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

okay

@agoscinski agoscinski merged commit 93f1b03 into main Apr 4, 2023
@agoscinski agoscinski deleted the docs_cleanup branch April 4, 2023 13:02
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