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

src/tox.ini: Add validation of docstrings using flake8-rst-docstrings #30448

Closed
mkoeppe opened this issue Aug 26, 2020 · 92 comments
Closed

src/tox.ini: Add validation of docstrings using flake8-rst-docstrings #30448

mkoeppe opened this issue Aug 26, 2020 · 92 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Aug 26, 2020

Short of running a single-file docbuild (which needs fixing - #30475), there are several options for validating rst files and docstrings:

Here we add a tox environment using https://pypi.org/project/flake8-rst-docstrings.

Usage:

   ./sage -tox -e rst src/sage/geometry/polyhedra

We also run it as part of the Lint workflow on GH Actions (see badge).

Continued in the meta ticket: #34157

Depends on #30503

CC: @tobiasdiez @jhpalmieri @dimpase @fchapoton @DaveWitteMorris @Etn40ff @tscrim @kwankyu

Component: documentation

Author: Matthias Koeppe

Branch/Commit: 0677319

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/30448

@mkoeppe mkoeppe added this to the sage-9.2 milestone Aug 26, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 29, 2020

Dependencies: #29651

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 30, 2020

Changed dependencies from #29651 to #29651, #30475

@tobiasdiez
Copy link
Contributor

comment:5

There are also linters for rst files: rstcheck or doc8

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 30, 2020

comment:6

Yes, see also my comments in #30415

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

comment:7

Replying to @mkoeppe:

Yes, see also my comments in #30415

... added to the description

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

Changed dependencies from #29651, #30475 to #29651

@mkoeppe mkoeppe changed the title src/tox.ini: Add validation of rst files and docstrings via a quick docbuild src/tox.ini: Add validation of rst files and docstrings Sep 7, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

Changed dependencies from #29651 to #30503

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

comment:11

As a first stab at docstring validation, I have added tox -e pydocstyle. This will need some configuration -- right now it gives just too many warnings.


Last 10 new commits:

ca64a52src/tox.ini (codespell): Skip non-English doc, binary files, backup files
d8e101csrc/.codespell-dictionary.txt: New, consider these words correct
e327314src/bin/sage (-advanced): Generalize tox environment list formatting
0847d49tox.ini: When delegating to src/tox.ini, use -- as a separator
2ff2985src/.codespell-ignore.txt: New; use in addition to .codespell-dictionary.txt
92c3b3dtox.ini (codespell): If invoked from SAGE_ROOT, codespell the whole Sage distribution by default
caba287src/tox.ini (codespell): Skip more files and directories
e5a916ftox.ini: Add codespell to envlist
c20fad1Merge branch 't/30503/apply_fossies_codespell' into t/30448/src_tox_ini__add_validation_of_rst_files_and_docstrings
10b6028WIP: src/tox.ini (pydocstyle): New

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2020

Commit: 10b6028

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@tobiasdiez
Copy link
Contributor

comment:14

For the automated check, it's also a problem that Sage uses a custom doc style and not one of the more standardized ones like pep257, numpy or google.
https://www.python.org/dev/peps/pep-0257/
https://numpydoc.readthedocs.io/en/latest/format.html
https://google.github.io/styleguide/pyguide.html#383-functions-and-methods

Are there any plans to migrate to one of these standards?

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 10, 2020

comment:15

I'm not aware of migration plans

@jhpalmieri
Copy link
Member

comment:16

Replying to @mkoeppe:

I'm not aware of migration plans

Nor am I, but it's something to consider.

@tobiasdiez
Copy link
Contributor

comment:17

I've created #31044 for this.

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 12, 2020

comment:18

Also if the decision is made to migrate, to do the actual migration we will certainly need a parser for the sage-style doc.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 24, 2021

comment:19

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@mkoeppe mkoeppe removed this from the sage-9.4 milestone Jul 19, 2021
@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

Changed dependencies from #30503; #34156 to #30503

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:59

Here is the meta ticket: #34157

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:60

Done. It is a ticket party. Whet your appetite.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:61

Perhaps

diff --git a/src/sage/graphs/domination.py b/src/sage/graphs/domination.py
index 25ac512233..8cc1e57de9 100644
--- a/src/sage/graphs/domination.py
+++ b/src/sage/graphs/domination.py
@@ -513,7 +513,7 @@ def _cand_ext_enum(G, to_dom, u_next):
 
         .. WARNING::
 
-            The same output may be output several times (up to ``H`` times).
+            The same output may be output several times (up to `|H|` times).
 
         In order to later remove duplicates, we here output pairs ``(ext, i)``
         where ``ext`` is the output candidate extension and ``i`` counts how
@@ -593,7 +593,7 @@ def minimal_dominating_sets(G, to_dominate=None, work_on_copy=True):
     - ``work_on_copy`` -- boolean (default: ``True``); whether or not to work on
       a copy of the input graph; if set to ``False``, the input graph will be
       modified (relabeled).
-    
+
     OUTPUT:
 
     An iterator over the inclusion-minimal sets of vertices of ``G``.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 12, 2022

comment:62

Thanks for catching this, please push it to the ticket

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

Changed commit from d66c66e to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2022

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

450cdc3src/sage/modular: Fix some errors shown by tox -e rst
74738fbsrc/sage/plot: Fix some errors shown by tox -e rst
dccceb4src/sage/parallel: Fix errors shown by tox -e rst
4cc5674src/sage/quadratic_forms: Fix errors shown by tox -e rst
a2f5ef6src/sage/repl: Fix some errors shown by tox -e rst
c2d0a91src/sage/rings: Fix some errors shown by tox -e rst
8fc8776src/sage/structure: Fix errors shown by tox -e rst
18121fesrc/sage/modules: Fix errors shown by tox -e rst
d66c66e.github/workflows/lint.yml: Do not fail when rst checks fail
0677319Fix a typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 12, 2022

Commit: 0677319

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

comment:65

Otherwise, it looks good to me.

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

Changed author from Matthias Koeppe, ... to Matthias Koeppe

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 12, 2022

Reviewer: Kwankyu Lee

@kwankyu

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 12, 2022

comment:67

Thanks!

@jhpalmieri
Copy link
Member

comment:68

A bug in flake8-rst-docstrings? See #34168 comment:14, also #34167 and #34169.

@jhpalmieri
Copy link
Member

comment:69

Or maybe we're just sloppy in how we use reStructuredText, and we should add explicit titles our files, like

diff --git a/src/sage/categories/category_with_axiom.py b/src/sage/categories/category_with_axiom.py
index 237ad5dfed..d11bf0fe8a 100644
--- a/src/sage/categories/category_with_axiom.py
+++ b/src/sage/categories/category_with_axiom.py
@@ -1,5 +1,7 @@
 r"""
+======
 Axioms
+======
 
 This documentation covers how to implement axioms and proceeds with an
 overview of the implementation of the axiom infrastructure. It assumes

I don't know rst syntax well enough to know if a single word at the start of a text block has special significance, but I think it might. This might also explain the issue with "noreplace" in sagedoc.py in #34172.

@jhpalmieri
Copy link
Member

comment:70

I can't get the syntax

======
Axioms
======

to compile: it yields an error. At #34168 we've diagnosed what looks like a bug in flake8-rst-docstrings, and fixing that would be my preferred resolution.

@vbraun
Copy link
Member

vbraun commented Jul 21, 2022

Changed branch from public/30448 to 0677319

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

No branches or pull requests

5 participants