search

twolfson / restructuredtext-lint

reStructuredText linter
The Unlicense
171 stars 20 forks source link

Allow optionally ignoring sphinx directives/roles #14

Closed harlowja closed 9 years ago

harlowja commented 9 years ago

Sphinx roles can create false alarms and false errors if we do not ignore them, so this adds the ability to ignore them (and turns it on by default).

Do note that when enabled this module will patch ignore directives and roles into the sphinx code-base and then make an attempt to remove them when it has finished (this is inherently not thread-safe).

Fixes issue #11

twolfson commented 9 years ago

I believe this is in the opposite of the intent of #11. We want to be able to support people to extend on top of restructuredtext-lint to add linting for Sphinx.

twolfson commented 9 years ago

Sorry, I mean that it is the opposite of my understanding of what we wanted from #11.

harlowja commented 9 years ago

Hmmm, good point/question. I'd almost rather just ignore them to start then maybe dig deeper into examining them, shall I point this at another issue then (maybe one called ignore sphinx directives with later plans to examine at a deeper level)?

twolfson commented 9 years ago

I am not entirely sure yet. I will have to think about this deeper. My gut reaction is to say only allow for hooks as ignoring can be an implementation default of a hook.

twolfson commented 9 years ago

Left some comments for the case of if we leverage this PR as the starting point for hooks.

twolfson commented 9 years ago

Okay, there are still a lot of comments that I have queued up in my head and I would like to avoid the whole back/forth on. I am going to commandeer the PR and stack some commits on top of yours to get this landed.

Regarding the comment way back about needing time to think about this. I believe the custom directives input with providing Sphinx defaults will work out for the best. On the CLI, we can include an --ignore-sphinx option to leverage that.

harlowja commented 9 years ago

Sounds good to me :)

twolfson commented 9 years ago

I have completed mostly everything except for CLI integration and documentation. However, now I am reviewing the Sphinx code base and questioning our intentions. Why aren't we running Sphinx's directives or instructing users to register their directives to docutils instead of doing everything through our library?

We will indefintely need them to use the docutils API for their directives so all of this seems futile.

We should be providing instructions on how to import Sphinx directives instead.

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/directives/code.py#L354-L358

Side note of what was changed since the last commit in this PR:

https://github.com/twolfson/restructuredtext-lint/tree/a8043d929b8f9fcb3a2218835caa1d0cc6e9e4bb/restructuredtext_lint

harlowja commented 9 years ago

As for the general question; that's up to u. Now that I know how to do this I could just do it in doc8, your branch/commit looks fine to me. I'll let u make the final decision on what u want to do here :-P

twolfson commented 9 years ago

I guess I will play around with it to see how docutils behaves once sphinx is loaded. If it still does lint errors for sphinx utils errors, then I think that is ideal. If it does something else, then it will likely depend on how it acts.

harlowja commented 9 years ago

Ok dokie.

twolfson commented 9 years ago

To properly set expectations, I will play around with it by the end of the weekend.

harlowja commented 9 years ago

No worries; its been sitting for a while anyway ;)

twolfson commented 9 years ago

I have great news! When we load the Sphinx directives in, they play perfectly with our lint errors =)

The downside is that while I could find some parts of Sphinx that added directives, I cannot seem to find how/where the BUILTIN_DOMAINS are registered to docutils. Can you be my second set of eyes and take a look in the repo?

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/domains/python.py#L582-L622

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/domains/__init__.py#L278-L292

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/application.py#L81

and then after we find self.domains, I cannot find any invocations of add_directive =(

harlowja commented 9 years ago

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/builders/html.py#L274 seems to be something (domains seems to be transferred into the 'env' variable). Perhaps the rabbit hole goes deeper from there...

harlowja commented 9 years ago

Also a bunch of domain stuff in https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py

twolfson commented 9 years ago

I decided to jump in from the perspective of running a Sphinx CLI program. It starts with cmdline, then picks a Builder (as you have linked), determines if there are files to update, and then builds the files in serial/parallel.

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/__init__.py#L92

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/cmdline.py#L242-L245

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/application.py#L263-L264

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/builders/__init__.py#L243-L245

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/builders/__init__.py#L315-L316

https://github.com/sphinx-doc/sphinx/blob/1.3.1/sphinx/builders/__init__.py#L360

I am still diving down...

twolfson commented 9 years ago

Alright, I think we have our answer. Sphinx doesn't use docutils for its directives. It implements on top of domains so we will have to roll our own utility to do this OR if it's simple enough, provide instructions on how to.

In these references, we see sphinx talking directly to domains about resolving xref:

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py#L1281-L1302

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py#L1527-L1569

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py#L1541-L1545

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py#L1633-L1674

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/environment.py#L1655-L1656

twolfson commented 9 years ago

I have hit another turning point in the rabbit hole. I was working on adding in testing for builtin domains (e.g. py:function::) and ran into Sphinx starting to leverage it's environment concept.

https://github.com/sphinx-doc/sphinx/blob/1.3/sphinx/application.py#L210-L216

This requires a whole setup of a configuraiton file and src/dest directories for it to work properly. If we want accurate linting, then we would have to absorb all of that into rst-lint. However, it starts to become something like a sphinx-lint at that point.

I think I am going to resolve this by documenting how to import basic sphinx directives which utilize docutils (e.g. code) via register_directive and close any deeper integration with sphinx as a wontfix.

twolfson commented 9 years ago

For historical purposes, here is the branch where all exploring was done:

https://github.com/twolfson/restructuredtext-lint/tree/09e7296de425826cc3f12e8d0fd14c8e3a2a1e60

twolfson commented 9 years ago

I have added documentation in 0.11.3. Closing this as a wontfix since we want accuracy but that leaves the scope of this repo since it becomes specific to sphinx, not reStructuredText. Thanks for all your help and quick responses on this issue =)

harlowja commented 9 years ago

Sounds good to me. Thanks!

julienvey commented 8 years ago

Commenting on this issue, because we currently use the CLI, and there is actually no workaround that I'm aware of to avoid issues with sphinx directives

twolfson commented 8 years ago

@julienvey Here are some options to consider:

peterjc commented 5 years ago

Hi @twolfson - can you point me at any restructuredtext-lint documentation changes that came out of this work?

I'm interested for two reasons, first I am already using your tool rst-lint at the command line to check *.rst files, and in some projects am looking at adopting Sphinx. I'll take a look at doc8 here.

Second, from the perspective of my flake8 plugin, https://github.com/peterjc/flake8-rst-docstrings - which uses your library to validate Python docstrings as RST. It sounds like your exploratory work might allow me to neatly solve https://github.com/peterjc/flake8-rst-docstrings/issues/7 ?

peterjc commented 5 years ago

In terms of the new documentation, was it just the highlight example in the main README?

# Load in our dependencies
from docutils.parsers.rst.directives import register_directive
from sphinx.directives.code import Highlight
import restructuredtext_lint

# Load our new directive
register_directive('highlight', Highlight)

# Lint our README
errors = restructuredtext_lint.lint_file('docs/sphinx/README.rst')
print errors[0].message # Error in "highlight" directive: no content permitted.

In terms of code, I think I could try re-using some of this work... https://github.com/twolfson/restructuredtext-lint/blob/09e7296de425826cc3f12e8d0fd14c8e3a2a1e60/restructuredtext_lint/sphinx.py

peterjc commented 5 years ago

Hmm, the dictionary BUILTIN_DOMAINS was removed in Sphinx 1.4

https://github.com/sphinx-doc/sphinx/commit/8a45aa5e59b87703803bfb44c696afd06b609335#diff-3579c7ee6bc5111e6a44925d209f6f4d

It looks like this tuple of strings is the replacement,

from sphinx.application import builtin_extensions

Quite a lot more has changed...

twolfson commented 5 years ago

Yep, that code example and the wording before it was the documentation added around Sphinx as well as a link to https://github.com/twolfson/restructuredtext-lint/issues/29#issuecomment-243456787