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

Fix error table RST, max short code now over 69 chars #396

Merged
merged 3 commits into from Aug 15, 2019

Conversation

@peterjc
Copy link
Contributor

peterjc commented Aug 12, 2019

Should close #380.

Thanks for submitting a PR!

Please make sure to check for the following items:

  • Add unit tests and integration tests where applicable.
    If you've added an error code or changed an error code behavior,
    you should probably add or change a test case file under tests/test_cases/ and add
    it to the list under tests/test_definitions.py.
    If you've added or changed a command line option,
    you should probably add or change a test in tests/test_integration.py.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

Please don't get discouraged as it may take a while to get a review.

peterjc added 2 commits Aug 12, 2019
@peterjc

This comment has been minimized.

Copy link
Contributor Author

peterjc commented Aug 12, 2019

If adding a dependency on https://pypi.org/project/restructuredtext-lint/ was acceptable, I could add a test for this?

@lordmauve

This comment has been minimized.

Copy link
Contributor

lordmauve commented Aug 12, 2019

There are different requirements files for testing and documentation from the package itself, so I would guess that adding a dependency to one of those would not be a problem: https://github.com/PyCQA/pydocstyle/tree/master/requirements

@peterjc

This comment has been minimized.

Copy link
Contributor Author

peterjc commented Aug 12, 2019

Adding this to the tox docs jobs makes sense to me, but looking at all the RST files, we might need to tweak rst-lint a bit first to check all the files: twolfson/restructuredtext-lint#52

$ python -c "from pydocstyle.violations import ErrorRegistry; print(ErrorRegistry.to_rst())" > docs/snippets/error_code_table.rst
$ rst-lint docs/*.rst docs/*/*.rst
ERROR docs/error_codes.rst:23 Unknown interpreted text role "ref".
ERROR docs/index.rst:19 Unknown directive type "toctree".
ERROR docs/snippets/config.rst:39 Unknown interpreted text role "ref".
ERROR docs/snippets/pre_commit.rst:232 Undefined substitution referenced: "version".
ERROR docs/snippets/config.rst:39 Unknown interpreted text role "ref".
ERROR docs/snippets/pre_commit.rst:6 Undefined substitution referenced: "version".

Installing restructuredtext-lint just for a couple of files seems rather heavy handed.

Perhaps running sphinx in a stricter mode would be better?

@lordmauve

This comment has been minimized.

Copy link
Contributor

lordmauve commented Aug 12, 2019

Perhaps running sphinx in a stricter mode would be better?

Yes - Sphinx's flavour of reStructuredText won't be understandable to plain docutils. I think Sphinx normally has a "dummy" build target that doesn't generate any output, but validates the input, but I don't see it in pydocstyle's docs Makefile.

@peterjc

This comment has been minimized.

Copy link
Contributor Author

peterjc commented Aug 12, 2019

It seems better as a separate pull request, but maybe:

$ sphinx-build -h
...
  -n                nit-picky mode, warn about all missing references
...
  -W                turn warnings into errors
@sigmavirus24

This comment has been minimized.

Copy link
Member

sigmavirus24 commented Aug 12, 2019

Right, nothing keeps the Makefile up-to-date and users can modify it. This is why Flake8 uses sphinx-build directly in its tox.ini to validate what it cares about in the docs. Might be helpful to crib from there.

@Nurdok

This comment has been minimized.

Copy link
Member

Nurdok commented Aug 15, 2019

I'll merge this in a few minutes when the build completes successfully (had to resolve release notes conflict).

@peterjc - I'm in favor of adding a linter to check for these kinds of errors in the future and don't mind depending on another package (for development / testing only, this is a no brainer). Feel free to send a followup PR for that.

Thanks!

@Nurdok
Nurdok approved these changes Aug 15, 2019
@Nurdok Nurdok merged commit 8a5376b into PyCQA:master Aug 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@peterjc peterjc deleted the peterjc:error_table branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.