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 docstrings markups #227

Merged
merged 5 commits into from
May 26, 2024
Merged

Cleanup docstrings markups #227

merged 5 commits into from
May 26, 2024

Conversation

PicoCentauri
Copy link
Collaborator

@PicoCentauri PicoCentauri commented May 2, 2024

While scrolling through the repository I found a lot of markup issues. They are not relevant for the rendered docs but triggered me a bit 🤣

I took the liberty to fix them and added a simple linter (flake8-docstrings) that will catch the very evil ones in the future.

The codebase is smaller and wherever I found I replaced ndarray -> numpy.ndarray to have an intersphinx link to the Numpy docs.

There is still a lot of space to imrprove the markup i.e. highlighting variables with double back ticks `` or using more intersphinx links in the docs. But lets keep it like this for now.


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

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 that cumbersome work!

I am only a bit worried that enforcing a linter without a formatter will be quite annoying. Did you manually format it or used some formatter as baseline? Referencing this issue #182

tox.ini Outdated
Comment on lines 104 to 112
ignore =
E203
D100
D101
D102
D205
D400
D401
W503
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just assume this makes sense

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 mean a lot of them are very useless. You can take a look if you disagree with the ones I disabled.

The ones I disabled mean

D100 Missing docstring in public module
D101 Missing docstring in public class
D102 Missing docstring in public method
D205 1 blank line required between summary line and description

D400 First line should end with a period
D401 First line should be in imperative mood

Every Code that we remove from this list will require further changes to the docstrings.

E203 and W503 have to be disabled due to incompatibilities with black.

@PicoCentauri
Copy link
Collaborator Author

I am only a bit worried that enforcing a linter without a formatter will be quite annoying. Did you manually format it or used some formatter as baseline? Referencing this issue #182

Everything is dine by Hand and most of the changes dictated by the linter. The things which are not catched and I changed are basically following the numpydoc which for us basiclally boils down to

  • no empty line after a section
  • no empty line after a parameter
  • consistent indentation of parameters of 4 spaces
  • spaces around delimiter colons :
  • using 88 chars as line width (for this I am using Rewrap plugin of VSCode)

this means I changed a docstring like this

    Parameters
    ----------

    mixing: float, default=0.5
    The PCovR mixing parameter, as described in PCovR as
    :math:`{\\alpha}`

    initialize: int or 'random', default=0
        Index of the first selection. If 'random', picks a random
        value when fit starts.

to this

    Parameters
    ----------
    mixing : float, default=0.5
        The PCovR mixing parameter, as described in PCovR as :math:`{\\alpha}`
    initialize : int or 'random', default=0
        Index of the first selection. If 'random', picks a random value when fit starts.

I agree that this is a bit unfortunate that there is no formatter for this but if we as reviewers know these rules One can easily comment in a PR.

@agoscinski
Copy link
Collaborator

Replaced now flake with ruff. With that most linter errors should be automatically fixed. Just so you know with ruff check there is also the --unsafe-fixes option that can fix additional errors but is unsafe. tox does not really allow us to expose this option in a nice way, but it could be temporary put into the tox.ini for development.

@PicoCentauri could you have a look at my commits and merge if it is okay for you?

Copy link
Collaborator Author

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Looks very good to me. Thanks for further improvmenets @agoscinski

PicoCentauri and others added 5 commits May 26, 2024 20:28
By replacing flake8 with ruff we can now automatically fix a lot of the
linter errors with `ruff check --fix`. Added ignores for D410 D411, to
get no additional linter errors.
`tox -e format-unsafe` allows to format unsafe that fixes more linter
errors but might alter code logic.
With that we can expose the contributer to `tox -av` in the contributers
guidelines.
@agoscinski agoscinski merged commit 9462c0b into main May 26, 2024
10 checks passed
@agoscinski agoscinski deleted the docs-linting branch May 26, 2024 18:49
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