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

New Option: Configurable RST Substitutions #62

Merged
merged 3 commits into from
Jun 7, 2022

Conversation

andthum
Copy link
Contributor

@andthum andthum commented Jun 7, 2022

Related to Issues #7 and #10 and PR #16.

As bskinn explained in #10, RST allows the definition of substition sequences. Commonly used substitutions can be put into rst_epilog (or rst_prolog) of Sphinx' conf.py file. However, RST does not know about these definitions in conf.py and thus flake8 throws an RST305 Undefined substitution referenced: "XXX" error.

You could add RST305 to the list of ignored error codes or ignore it via flake8's per-file-igonres, like bskinn did. However these ways of ignoring the error code are quite coarse and you would not be warned if you indeed referenced an undefined substitution in one of the files.

Therefore, I suggest to add a config option rst-substitutions where users can enter self-defined substitutions to suppress error code RST305, similar to rst-directives and rst-roles introduced in #16.

Allow to add user-defined RST substitutions that don't throw error code
RST305.
@andthum andthum force-pushed the feat/option/extra-substitutions branch from 153c276 to 577f17a Compare June 7, 2022 13:47
@peterjc
Copy link
Owner

peterjc commented Jun 7, 2022

This sounds reasonable.

Note to self: If we switch the backend to rstcheck (now that the v6 API changes are done), that has options for this (and roles and directives) which we should be able to use rather than silencing errors (available at the command line and probably via Python too).

@andthum
Copy link
Contributor Author

andthum commented Jun 7, 2022

Glad to hear that you find it reasonable.

I hope I did everything correctly. I'm not sure how to handle the Code Climate error.

@peterjc
Copy link
Owner

peterjc commented Jun 7, 2022

I'm not too bothered about the CodeClimate warning. We could combine the three if statements into a single if ... and ... and ...: return 0 but that would probably be less readable.

What I would like to see is at least one test case using the new functionality though, something like the roles and directives test in tests/test_all.py would be enough.

andthum added 2 commits June 7, 2022 17:04
Add a test case to `test_all.py` that checks whether ignoring undefined
substitutions (RST305) works.
@andthum andthum force-pushed the feat/option/extra-substitutions branch from 3f11d43 to 0208203 Compare June 7, 2022 15:04
@andthum
Copy link
Contributor Author

andthum commented Jun 7, 2022

Just tell me if you want to have more test cases (now there is really only one test case) or if you want anything else to be done.

@peterjc
Copy link
Owner

peterjc commented Jun 7, 2022

That looks good, and worth calling this the v0.2.6 release 👍

@peterjc peterjc merged commit 0d9b6f4 into peterjc:master Jun 7, 2022
@peterjc
Copy link
Owner

peterjc commented Jun 7, 2022

Released, thank you.

@andthum
Copy link
Contributor Author

andthum commented Jun 7, 2022

Thank you for accepting my PR and your fast respond 😉

@andthum andthum deleted the feat/option/extra-substitutions branch June 7, 2022 16:26
@peterjc
Copy link
Owner

peterjc commented Jun 7, 2022

I forgot to ask, would you like to be acknowledged in the release notes (see previous examples)?

@andthum
Copy link
Contributor Author

andthum commented Jun 7, 2022

Oh, this would be kind. Yes, thank you!

peterjc added a commit that referenced this pull request Jun 8, 2022
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 this pull request may close these issues.

2 participants