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

Gives errors on well known text roles #7

Closed
Thomasillo opened this issue Jun 27, 2018 · 21 comments · Fixed by #16
Closed

Gives errors on well known text roles #7

Thomasillo opened this issue Jun 27, 2018 · 21 comments · Fixed by #16

Comments

@Thomasillo
Copy link

like :class: or :func:

@peterjc
Copy link
Owner

peterjc commented Jun 27, 2018

Could you give a complete example? Where are these roles used? Are they Sphinx specific?

@Thomasillo
Copy link
Author

Assume a file example.py

""":class:`X` is a good class."""

Then

$ flake8 example.py
example.py:2:1: RST304 Unknown interpreted text role "class".

sphinx specific, yes, I think so.

@peterjc
Copy link
Owner

peterjc commented Jun 29, 2018

This was added to resolve #4, with the expectation that you'd ignore RST304.

Is there a (finite) list of these text roles which you'd like to accept as valid?

@Thomasillo
Copy link
Author

IMHO, ignoring RST304 is not an option. Then the linter would not bark on mistakes like

""":calss:`X`"""

anymore.

However, being able to give a list of allowed text roles (e.g. in the setup.cfg) would definitely work. Is that possible already?
What would be needed for this?
I couldn't find anything like that in the doc.

@peterjc
Copy link
Owner

peterjc commented Jun 29, 2018

Docutils describes a list of known roles here http://docutils.sourceforge.net/docs/ref/rst/roles.html

  • :emphasis:
  • :literal:
  • :code:
  • :math:
  • :pep-reference:
  • :rfc-reference:
  • :strong:
  • :subscript:
  • :superscript:
  • :title-reference:
  • and the special case raw

This document also describes how roles are defined with a directive, but the URL for that seems not to be working, http://docutils.sourceforge.net/docs/ref/rst/directives.html#role

According Sphinx documentation about roles http://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html this does not seem to define a class role (as in the bug report). Only the following:

  • :any:
  • :ref:
  • :doc:
  • :download:
  • :numref:
  • :envvar:
  • :token:
  • :keyword:
  • :option:
  • :term:
  • :abbr:
  • :command:
  • :dfn:
  • :file:
  • :guilabel:
  • :kbd:
  • :mailheader:
  • :makevar:
  • :manpage:
  • :menuselection:
  • :mimetype:
  • :newsgroup:
  • :program:
  • :regexp:
  • :samp:
  • :pep:
  • :rfc:

Can you configure additional roles in Sphinx? If so, there is a mechanism in flake8 for plugins to have settings like this, and that would be the best way to provide a list of known/expected text roles.

Thus far this plugin has not needed any settings, so this aspect of a flake8 plugin is something I have not looked into. I'm happy to look at pull request implementing this though.

@Thomasillo
Copy link
Author

In the sphinx documentation it is described here:

http://www.sphinx-doc.org/en/stable/domains.html#cross-referencing-python-objects

As python is the default domain :py:class: can be abbreviated to :class: and likewise for func, mod, ...

AFAIK, this is standard, when cross-referencing to objects of your codebase (or outside with intersphinx) within the documentation.

I will try to look into the configurability and see if I can do something :)

@peterjc
Copy link
Owner

peterjc commented Jul 6, 2018

The list of text roles in Sphinx seems very long, but if we can pull out a default white list, this does seem worth it to catch typos like your example:

:calss:`X`

which should have been

:class:`X`

I wonder if there is a way to import the list of defined roles from Sphinx at run time?

@peterjc
Copy link
Owner

peterjc commented Nov 14, 2018

In code this gives some lists:

>>> from sphinx.application import Sphinx
>>> s = Sphinx('.', None, '.', '.', 'html')
Running Sphinx v1.6.6
loading pickled environment... not yet created
>>> sphinx_roles = set(role for domain in s.registry.domains.values() for role in domain.roles)
>>> sphinx_directives = set(directive for domain in s.registry.domains.values() for directive in domain.directives)
>>> print("Loaded %i roles and %i directives from Sphinx defaults" % (len(sphinx_roles), len(sphinx_directives)))
Loaded 27 roles and 32 directives from Sphinx defaults
>>> for _ in sorted(sphinx_roles): print(_)
... 
any
attr
class
concept
const
data
dir
doc
enum
enumerator
envvar
exc
func
keyword
macro
member
meth
mod
numref
obj
option
ref
role
term
token
type
var
>>> for _ in sorted(sphinx_directives): print(_)
... 
attribute
class
classmethod
cmdoption
concept
currentmodule
data
decorator
decoratormethod
directive
enum
enum-class
enum-struct
enumerator
envvar
exception
function
glossary
macro
member
method
module
namespace
namespace-pop
namespace-push
option
productionlist
program
role
staticmethod
type
var

@peterjc
Copy link
Owner

peterjc commented Nov 14, 2018

In fact, focusing that down we probably would be better with this subset as defaults (ignoring C, C++ and JavaScript, taking just the standard set, RST, and Python ones):

>>> from sphinx.application import Sphinx
>>> s = Sphinx('.', None, '.', '.', 'html')
Running Sphinx v1.6.6
loading pickled environment... not yet created
>>> sphinx_roles = set()
>>> sphinx_directives = set()
>>> for d in ('py', 'rst', 'std'):
...     domain = s.registry.domains[d]
...     for role in domain.roles:
...         sphinx_roles.add(role)
...         sphinx_roles.add(d + ':' + role)
...     for dir in domain.directives:
...         sphinx_directives.add(dir)
...         sphinx_directives.add(d + ':' + dir)
... 
>>> print(sorted(sphinx_roles))
['attr', 'class', 'const', 'data', 'dir', 'doc', 'envvar', 'exc', 'func', 'keyword', 'meth', 'mod', 'numref', 'obj', 'option', 'py:attr', 'py:class', 'py:const', 'py:data', 'py:exc', 'py:func', 'py:meth', 'py:mod', 'py:obj', 'ref', 'role', 'rst:dir', 'rst:role', 'std:doc', 'std:envvar', 'std:keyword', 'std:numref', 'std:option', 'std:ref', 'std:term', 'std:token', 'term', 'token']
>>> print(sorted(sphinx_directives))
['attribute', 'class', 'classmethod', 'cmdoption', 'currentmodule', 'data', 'decorator', 'decoratormethod', 'directive', 'envvar', 'exception', 'function', 'glossary', 'method', 'module', 'option', 'productionlist', 'program', 'py:attribute', 'py:class', 'py:classmethod', 'py:currentmodule', 'py:data', 'py:decorator', 'py:decoratormethod', 'py:exception', 'py:function', 'py:method', 'py:module', 'py:staticmethod', 'role', 'rst:directive', 'rst:role', 'staticmethod', 'std:cmdoption', 'std:envvar', 'std:glossary', 'std:option', 'std:productionlist', 'std:program']

Those lists are with and without the namespace prefixes.

The simplest solution is to hard code these values (and later make them configurable), and then filter the warnings accordingly. This would catch typos at least.

The potential clever solution is to tell docutils about the Sphinx directives, and that might actually validate their usage as well?

rhertzog added a commit to rhertzog/distro-tracker that referenced this issue Nov 25, 2018
This plugin ensures that docstrings are valid reStructuredText.
It is not fully compliant with Sphinx's usage so we temporarily
ignore its RST304 tag until
peterjc/flake8-rst-docstrings#7 is fixed.
@bskinn
Copy link
Contributor

bskinn commented Apr 22, 2019

IMO it's not particularly useful to try to enhance flake8-rst-docstrings to catch typos like :class: --> :calss:, since these can be raised as errors during a nitpicky Sphinx build.

Looks like the changes are already underway, though?

@peterjc
Copy link
Owner

peterjc commented Apr 22, 2019

I can see how to do it (see #8), but not having a personal motivating example makes it a bit tricky to see how best to design the configuration and defaults. Thoughts welcome here.

@bskinn
Copy link
Contributor

bskinn commented Apr 25, 2019

I would actually argue against trying to augment flake8-rst-docstrings to do this kind of proofreading. It would be a massive undertaking to try to develop and curate a list of valid roles, whether manually or automatically, especially because (1) some of them might change with new Sphinx versions and (2) there is a universe of Sphinx extensions, any of which could add new roles, etc.

IMO a better alternative is to run Sphinx builds with the 'nitpicky' option (-n); with this option enabled, any errors in roles, references, substitutions, etc. are reported as warnings during build. This is a much more reliable way to proofread such things, as Sphinx intrinsically knows all about its current configuration at build time.

On Linux, and probably also MacOS, in the newest versions of Sphinx one can do:

$ make html O=-n

On Windows, the machinery currently isn't as good; you need to either set %SPHINXOPTS% before running make html, or edit your make.bat to work with an additional CLI parameter as I've done here (by appending %2), and then run with make html -n.

@sobolevn
Copy link

@bskinn I agree with your arguments about sphinx roles. But, still: without this feature flake8-rst-docstrings is not usable with sphinx in the same project.

Since docstrings will have a lot of RST303 and RST304 violations. They can be ignored, but it is still not desired.

@peterjc
Copy link
Owner

peterjc commented Jun 25, 2019

It is useable (ignore RST303 and RST304 violation), but not ideal.

Automatically tracking the Sphinx ignore list might be possible, but seems overly complex and fragile - especially as there is so much potential end user configuration on any given Sphinx project.

I think an ignore-extend style setting is probably the best solution, default empty. We can document a typical Sphinx list (based on the work above), or users can grow it based on their own usage.

@sobolevn
Copy link

Yeap, sounds great!

@bskinn
Copy link
Contributor

bskinn commented Jun 25, 2019

Agreed, this sounds like a really good middle-ground approach. Most projects will only use a fraction of all the built-in Sphinx roles anyways, and so an ignore list should be pretty easy to maintain.

@peterjc
Copy link
Owner

peterjc commented Aug 7, 2019

Hello all (@Thomasillo, @sobolevn, @bskinn and anyone else following this), would anyone have a chance to look at my proposal on #16, which would resolve this with user configurable lists of additional reStructuredText directives and roles? I've included an example using this on numpy (which defines quite a few directives - but still a fairly small set).

[The fact that I've finally put my time into implementing this is driven by using Sphinx with apidoc for a work project - and finally having made some progress on moving Biopython towards using RST and Sphinx with apidoc for all of its main documentation]

@bskinn
Copy link
Contributor

bskinn commented Aug 7, 2019

<grin> Nothing like personal necessity to drive new features. 😄

@peterjc
Copy link
Owner

peterjc commented Aug 7, 2019

Release v0.0.11 is up on PyPI, conda-forge to follow. Thanks for the comments & feedback everyone.

@jlumbroso
Copy link

Thank you @peterjc for your stewardship!! Over a year later, I am enjoying finding in this thread a really nice example of constructive problem-solving in the context of open-source development. You deserve a lot of credit!

@peterjc
Copy link
Owner

peterjc commented Jan 3, 2021

Nice of you to say so @jlumbroso - This took a year to be resolved (sadly), but it the community input was invaluable in getting this resolved neatly.

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 a pull request may close this issue.

5 participants