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

[DOC] Fix invalid use of single-grave in docstrings #6023

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

geetu040
Copy link
Contributor

The invalid use of single-grave in docstrings has been fixed to correctly render code snippets in API Reference Documentation

Reference Issues/PRs

This PR relates to these comments

#5932 (comment)

code snippets do not render correctly (use single apostrophe instead of double)

#5984 (review)

Btw, some docstrings use single-apostrophe (grave) by error instead of double-apostrope (double-grave) to render code snippets. If you encounter these, it would also be great to replace them.

The confusion is due to markdown havin single-grave for code snippets, but rst has double-grave.

What does this implement/fix? Explain your changes.

  • In this PR, I have fixed the invalid use of single-grave at all places in the code.
  • This step is necessary as it enables the code snippets to render properly in the API Reference Documentation.
  • Although there are some valid uses of single-grave in the docstring (math expressions) that I have not changed.
  • Here is the Script on Google Colab that I used to auto-fix the docstrings

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

  • There are minor changes (just addition of single-grave) but at many lines in many files
  • Some lines exceeded the character link so I split them into 2 following the PEP 8 guidelines

Did you add any tests for the change?

No, Although I am planning to add one with some guidance

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.
    See here for further details on the algorithm maintainer role.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

@fkiraly fkiraly added the documentation Documentation & tutorials label Feb 27, 2024
@fkiraly
Copy link
Collaborator

fkiraly commented Feb 27, 2024

This is monumental! Nice use of regex!

Would you be able to contribute your tools to sktime? Not sure what the best place is, probably the build_tools section.
From there we could later convert it to a custom linting utility.

.all-contributorsrc Outdated Show resolved Hide resolved
The invalid use of single-grave in docstrings has been fixed to correctly render code snippets in API Reference Documentation
@geetu040
Copy link
Contributor Author

This is monumental! Nice use of regex!

Would you be able to contribute your tools to sktime? Not sure what the best place is, probably the build_tools section. From there we could later convert it to a custom linting utility.

Sure, I'll look into that

@geetu040
Copy link
Contributor Author

geetu040 commented Mar 1, 2024

Hi @fkiraly, could you answer me a few questions here:

  1. Should the test cover all the files in the repository for docstrings? If not then how should the docstrings of particular modules/classes/functions be called?
  2. Should the checks be in build_tools or as a test file for pytest. I think pytest makes more sense as initially tests are going to fail till all the code is adopted for double-grave. Also, we cannot auto-format the double-grave in all places because at some instances it requires manual fix.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 1, 2024

Hi @fkiraly, could you answer me a few questions here:

Sure! Just to say this quickly, I'd merge this separately, and imo we should move testing etc to another PR.

Should the test cover all the files in the repository for docstrings? If not then how should the docstrings of particular modules/classes/functions be called?

I would say, yes. Or, do you anticipate issues with some files?

Should the checks be in build_tools or as a test file for pytest. I think pytest makes more sense as initially tests are going to fail till all the code is adopted for double-grave. Also, we cannot auto-format the double-grave in all places because at some instances it requires manual fix.

Hm, not sure. I though that the "easiest" would be to just dump your code in build_tools for now, from where it can be manually executed, or later adapted.

Turning this into a test - or pre-commit step - requires more work. But, if you feel it is not too much work in addition, a pre-commit step or pytest test would work. If it requires a lot of work, then perhaps other items in sktime might be better payoff for time invested.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Hm, it would appear that the bulk fix also has affected some error message, e.g., quantiles must be N etc. It is odd that some error messages had backticks, I've removed then - let's hope that tests pass.

Might be worth thinking about for automatization.

@geetu040
Copy link
Contributor Author

geetu040 commented Mar 4, 2024

I would say, yes. Or, do you anticipate issues with some files?

No issues with the file
When I said all files I meant to check every python script for invalid single-grave in the project, including the test files, private objects and other elements that are not part of documentation. Should I cover them all, use a filter to avoid some or apply only on the docstring that will be a part of documentation because that is where we had the rendering issue initially

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 6, 2024

Yes, private elements should probably be excluded.

Anyway, that's probably for another PR, I'll merge this once I've looked through the files.

@fkiraly fkiraly merged commit 940777d into sktime:main Mar 6, 2024
230 of 231 checks passed
geetu040 added a commit to geetu040/sktime that referenced this pull request Mar 12, 2024
@duydl
Copy link
Contributor

duydl commented Mar 18, 2024

Hi. I stumbled on this while looking at blame. Just want to note that while it is best practice to have a double apostrophes for variable, we could also have a setting for single apostrophes to render similarly.

Edited:

The default role (content) has no special meaning by default. You are free to use it for anything you like, e.g. variable names; use the default_role config value to set it to a known role

https://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html

@geetu040
Copy link
Contributor Author

I get your point, you are saying that we can render single-apostrophes if we use sphinx roles instead of double-apostrophes

@duydl
Copy link
Contributor

duydl commented Mar 18, 2024

Yeah. And there is a global default_role config for sphinx to render single-apostrophes without any role in docstring. We could probably set it to render similarly to the double one.

@geetu040
Copy link
Contributor Author

double-apostrophes vs sphinx default role
I don't think we can say one is better than other can we?

@duydl
Copy link
Contributor

duydl commented Mar 18, 2024

Double-apostrophes is definitely better and we should aim for the standard. I just think it is wasteful to not set default role to something useful.

Edit: anyway I just gonna present that perhaps relevant. Hope it was informative somehow. I think your work on automation of docstring format would be really helpful. And sorry if it was distracting.

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 19, 2024

discussions like this are better in new issues, closed PR are rarely monitored.

Very valuable points, but I doubt anyone except us has seen these!

My recommendation on "how should have been" and on similar cases is:

  • open a new issue, framing the suggestions and design question
  • link to the pull request
  • discuss there

That way, we ensure that more people see it and contribute - and the useful content does not get lost or forgotten!

@fkiraly
Copy link
Collaborator

fkiraly commented Mar 19, 2024

(I think it still makes sense to open a new issue, at least to bring it to attention of other core devs)

fkiraly pushed a commit that referenced this pull request Mar 20, 2024
#### Reference Issues/PRs

This PR is related to this work here:
#6023

#### What does this implement/fix? Explain your changes.

It creates a python script in `build_tools` that can be run to list out
all the files and incorrect uses of `backticks` in the docstrings.

#### Any other comments?

If this tool is accepted we can use this to fix all the remaining
invalid `backtick` uses and can later be converted to a pre-commit tool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants