simplistix / sybil

Automated testing for the examples in your documentation.
https://sybil.readthedocs.io/en/latest/
Other
69 stars 14 forks source link

AttributeError on bad skip comment #140

Open adamtheturtle opened 4 days ago

adamtheturtle commented 4 days ago

Add this comment to a rST file:

.. skip<:

and use the sybil.parsers.rest.skip import SkipParser, and you will see:

../../../.virtualenvs/vws-python/lib/python3.12/site-packages/sybil/document.py:53: in parse
    for region in parser(document):
../../../.virtualenvs/vws-python/lib/python3.12/site-packages/sybil/parsers/abstract/skip.py:28: in __call__
    yield Region(lexed.start, lexed.end, match.groups(), self.skipper)
E   AttributeError: 'NoneType' object has no attribute 'groups'
cjw296 commented 4 days ago

Interesting :-) Fancy working up a PR to give a better error message?

adamtheturtle commented 4 days ago

I'm not against contributing but I'm not sure I can. First off, I'd like to have a shared idea of a goal. It probably would be no error message - my thinking is that this comment is not a Sybil comment / a comment Sybil should be looking for at all, so there should be no error (and nothing should be skipped).

cjw296 commented 4 days ago

Did you hit this intentionally or by a typo?

adamtheturtle commented 4 days ago

On the topic we were discussing before, I was trying to add custom skips for particular Sybils. I wanted a general skip of .. doccmd skip: next and a more specific skip of e.g. .. doccmd skip[type-checking]: next. I hit this issue while developing that, and then narrowed down the problem to what I have described in this issue.

adamtheturtle commented 4 days ago

Even if it were a typo, Sybil doesn't pick up on .. skop: next (untested), and I don't think it should.

cjw296 commented 1 day ago

There is a balance here: if something is structurally valid but omitted because it is of a directive type that we're not interested, then sure, no error.

If it's not a valid directive, or otherwise contains an error that the author is likely to have made by mistake, then we probably want to let them know. Sure, it'll become apparent soon enough that a directive has not been executed, but in cases where we can give a helpful exception sooner, I think Sybil should.

In this case its more nuanced, as skip and a couple of other Sybil things use pseudo-directives so as not to cause errors and warnings with other tools that process this text, particularly Sphinx.

The exception you originally mentions should be saying to the user "hey, I found a skip directive, but I couldn't parse it's arguments"

adamtheturtle commented 1 day ago

I believe that if this remains an error, it is impossible in Sybil to have two skip decorators, one .. doccmd skip: next and one .. doccmd skip[type-checking]. For my purposes, I've gone with .. doccmd skip[all] and .. doccmd skip[type-checking], but this limitation of the parsing logic was not expected by me.

cjw296 commented 1 day ago

The thing I'd say here: would .. doccmd skip[type-checking]:: be a valid ReST directive? If so, then Sybil should support the same structure as a pseudo-directive.

If not, then the above exception could be polished to be more helpful.

Any reason not to use .. doccmd-skip-all and .. doccmd-skip-typing? (suggesting those as they seem obvious valid in terms of being ReST directives)

adamtheturtle commented 1 day ago

I think what you're saying is that this is looking for something that is like a valid ReST directive but with only one : instead of two. I hadn't considered that! I was thinking of this only as a custom Sybil comment structure. My intention is that, like with skip: next, doccmd skip works the same across Markdown, ReST, MyST (and whatever comes next... djot, ...?).

I'm not sure if .. doccmd skip[type-checking]:: is a valid ReST directive, but I can look into it if that is important.

My goal is to allow users of the doccmd users specify --skip-marker options to enable skipping specific blocks. I was thinking of it as something like mypy, which has type: ignore which covers everything, as well as type:ignore[specific-code-1, specific-code-2]. I think your suggestion doesn't combine as well, but I'll consider it.

Please let me know if we're going too far into my personal use case!