rst-directive-colons has type text instead of rst, unlike the other two rst hooks

[CAM-Gerlach]

I'm not if it might be intentional (I don't see an indication of it), but the rst-directive-colons hook has types: [text] instead of rst like the other two reST hooks. Therefore, if unintentional, false positives might result, and it also doesn't show up when rst is selected on the pre-commit hooks page (by the way, was that dropdown a relatively new addition or did I just miss it before? Its really nice to have, thanks, and its a further motivation for me to help upgrade older hooks to use types instead of files wherever practicable).

Happy to submit a PR to fix this if it isn't intended, but I wanted to ask first. Thanks!

[asottile]

yeah I think this one should be types: [rst] as well -- a false positive for this is probably rare, but definitely possible

one counter argument that I could think of is matching stuff inside python docstrings, but in that case I think types_or: [rst, python] would make much more sense

looking at the other ones, the double-backticks one would false positive way too much on just text so that one shouldn't change, though I could see an argument for the other one being types_or: [rst, python] as well. though it's probably best to just set them all to types: [rst]

there doesn't seem to be any conversation about this so I probably just missed it while reviewing #33 -- CC @MarcoGorelli

[MarcoGorelli]

Hi all,

one counter argument that I could think of is matching stuff inside python docstrings

Yes, that's exactly my use-case. I put text because in pandas we wanted to check these on Python and markdown files, but types_or wasn't available yet. We don't get any false positives from having

-   repo: https://github.com/pre-commit/pygrep-hooks
    rev: v1.7.0
    hooks:
      - id: rst-backticks
      - id: rst-directive-colons
        types: [text]
      - id: rst-inline-touching-normal
        types: [text]

Having said that, types_or: [rst, python] does seem like a better fit now that it's available

[CAM-Gerlach]

Thanks for the detailed consideration, @asottile and @MarcoGorelli ! For Spyder, we keep our user docs, generated API docs, etc in separate repos from the code, and don't use much rst in our docstrings, so we wouldn't run this hook against our code repos, and only use it with rst against our docs repos (which have a conf.py and some Python build/deployment scripts, but no rst in docstrings). So, just rst would be slightly more optimal for us for performance reasons, but I certainly see the use case for rst, python.

The latter has the edge of ensuring wider check coverage by default, if false positives aren't a concern; on the other hand, there is some (probably small) performance cost and it requires a newer version of Pre-Commit. Ultimately, I'd go with whatever is consistent between hooks and matches user expectations for the default (i.e. least surprise). But maybe I'm overthinking this, since its easy to modify the types on the repo level so users can decide what works best for them (and I routinely do).

My proximate motivation for this issue is actually making sure the hooks are easily discoverable in the hook list on the pre-commit site (and by other tooling), and either fulfills that requirement (though having a rst hook show up in the python list as well may not be entirely intuitive).

[CAM-Gerlach]

@asottile @MarcoGorelli Any objection to going ahead with a PR to make rst-directive-colons and rst-inline-touching-normal include types_or: [rst, python] (or whatever else you agree on)? Thanks!

[asottile]

I think types: [rst] is probably best for now

[MarcoGorelli]

no objection, we'll override it on our end

[CAM-Gerlach]

Thanks, opened as PR #51