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.

Sign up for GitHub

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

Improve the contributing guidelines. #760

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Improve the contributing guidelines. #760

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0dae824
Improve the contributing guidelines.
tristanlatr Jan 2, 2024
2c56c8b
Fix mypy
tristanlatr Jan 2, 2024
99f304f
move mypy deps into the setup.cfg file
tristanlatr Jan 2, 2024
0bfa77e
Update docs/source/contrib.rst
tristanlatr Jan 3, 2024
034c765
Add a note about coding style.
tristanlatr Jan 17, 2024
abb5b87
Merge branch 'master' into 753-contrib-docs
tristanlatr Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions docs/source/contrib.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,56 @@ If you like the project and think you could help with making it better, there ar
Any contribution would be of great help and I will highly appreciate it! If you have any questions, please create a new issue.


Pre-commit checks
-----------------
Development process
-------------------

Make sure all the tests pass and the code pass the coding standard checks::
Create a fork of the git repository and checkout a new branch from ``master`` branch.
The branch name may start with an associated issue number so that we can easily
cross-reference them. For example, use ``1234-some-brach-name`` as the name of the branch working to fix issue ``1234``.
Once you're ready to run a full batterie of tests to your changes, open a pull request.

tox -p all
Don't forget to sync your fork once in while to work from the latest revision.

That should be the minimum check to run on your local system.
Pre-commit checks
-----------------

Make sure all the unit tests pass and the code pass the coding standard checks.

We use `tox <https://tox.wiki/en/stable/>`_ for running our checks, but you can roughly do the same thing from your python environment.

.. list-table:: Pre-commit checks
:widths: 10 45 45
:header-rows: 1

* - \
- Using `tox`
- Using your environment
* - Run unit tests
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
- ``tox -e test``
- ``pip install '.[test]' && pytest pydoctor``
* - Run pyflakes
- ``tox -e pyflakes``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you run a linter. You #757 you told me that you don't.

#757 is not fixed. IMHO it would be a good idea to mention your code style even if you do not care about code style. Also the latter is a "tell".

What I do not understand is why pyflakes do not throw any errors. I couldn't find a pyflakes config somewhere. But the code do not follow PEP8 and should throw 100 of errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only about line 47-48.

Copy link
Contributor Author

@tristanlatr tristanlatr Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant we don't have a formatter, like black. pyflakes check for errors in the code, not the formatting.

Copy link
Contributor

@buhtz buhtz Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"errors in the code" and "formatting" in Python is often the same because the format (e.g. indention) is part of the logic.

Anyway: I mixed up "pyflakes" with "flake8". Both are linters but "pyflakes" is of the "lazy type" and by design do not check for PEP8 or similar problems. Now I understand why it runs without errors. It is kind of a "minimal linter".

What kind of "errors" would pyflakes find? It's docu do not list errors.

But even this should be an element of the contrib docu. "No explicit coding style just following the minimal rules of pyflakes" (or something like this). But don't you have rules about naming variables for example; camel case, upper case letters only, etc pp? Do you accept PRs with line length of 120 characters?
The key of such pedantic details is that some contributors won't start a PR when they have to be scared that the maintainer will reject the PR because of (unknown) style reasons. Maintainers should state very clear what they want and what not. Otherwise contributors wouldn't risk to burn their resources.

Copy link
Contributor Author

@tristanlatr tristanlatr Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pyflakes will catch unused imports, name error and other relatively obvious mistakes. For the coding style I think we can just link the PEP8 guidelines. Pydoctor is about 20 years old, and we have vendored some code from other packages as well: so we can’t really enforce the same style everywhere. I’ll accept long line if it’s justified, it’s up to the reviewer to request refactors if some proposed change is very ugly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice explanation and could fit well into a contrib file.

- ``pip install pyflakes && find pydoctor/ -name \*.py ! -path '*/testpackages/*' ! -path '*/sre_parse36.py' ! -path '*/sre_constants36.py' | xargs pyflakes``
* - Run mypy
- ``tox -e mypy``
- ``pip install '.[mypy]' && mypy pydoctor``
* - Run pydoctor on it's own source
- ``tox -e apidocs``
- ``pip install . && pydoctor --privacy "HIDDEN:pydoctor.test" -q -W pydoctor``

These should be the minimum check to run on your local system.
A pull request will trigger more tests and most probably there is a tox
environment dedicated to that extra test.

Other things hapenning when a PR is open
----------------------------------------

- System tests: these tests checks if pydoctor can generate the documentation for a few
specific packages that have been considered as problematic in the past.
- Pydoctor primer: this is to pydoctor what ``mypy_primer`` is to ``mypy``.
It runs pydoctor on a corpus of open source code and compares the output of the application before and after a modification in the code.
Then it reports in comments the result for a PR. The source code of this tool is here: https://github.com/twisted/pydoctor_primer.
- Readthedocs build: For every PR, the sphinx documentation is built and available at ``https://pydoctor--{pr-number}.org.readthedocs.build/en/``.

Review process and requirements
-------------------------------
Expand Down
1 change: 1 addition & 0 deletions docs/source/quickstart.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ Pydoctor is a pretty verbose tool by default. It’s quite unlikely that you get
But don’t worry, pydoctor should have produced useful HTML pages no matter your project design or docstrings.

Exit codes includes:

- ``0``: All docstrings are well formatted (warnings may be printed).
- ``1``: Pydoctor crashed with traceback (default Python behaviour).
- ``2``: Some docstrings are mal formatted.
Expand Down
2 changes: 1 addition & 1 deletion pydoctor/epydoc/markup/restructuredtext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

from docutils.core import publish_string
from docutils.writers import Writer
from docutils.parsers.rst.directives.admonitions import BaseAdmonition # type: ignore[import]
from docutils.parsers.rst.directives.admonitions import BaseAdmonition # type: ignore[import-untyped]
from docutils.readers.standalone import Reader as StandaloneReader
from docutils.utils import Reporter
from docutils.parsers.rst import Directive, directives
Expand Down
15 changes: 15 additions & 0 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,21 @@ test =
Sphinx
pytest-subtests

mypy =
mypy>=0.902
mypy-zope
typing-extensions
; Libraries which include type annotations:
hypothesis
pytest>=6.0.0
sphinx>=3.4.0
twisted @ git+https://github.com/twisted/twisted.git
types-requests
; FIXME: https://github.com/twisted/pydoctor/issues/504
; This is pinned for now as newer versions are breaking our static checks.
types-docutils==0.17.5
types-toml

[options.entry_points]
console_scripts =
pydoctor = pydoctor.driver:main
Expand Down
19 changes: 4 additions & 15 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ commands =
[testenv:pyflakes]
description = Run pyflakes over the pydoctor code

deps: pyflakes
deps =
pyflakes

commands =
; Custom pyflakes run to exlcude some files. The demo packages are trigerring some unused imports warnings. But pydoctor uses those imports to resolve links. So we skip pyflakes for the demo sites. But not the test files.
Expand Down Expand Up @@ -182,20 +183,8 @@ commands =
[testenv:mypy]
description = run mypy (static type checker)

deps =
mypy>=0.902
mypy-zope
typing-extensions
; Libraries which include type annotations:
hypothesis
pytest>=6.0.0
sphinx>=3.4.0
git+https://github.com/twisted/twisted.git
types-requests
# FIXME: https://github.com/twisted/pydoctor/issues/504
# This is pinned for now as newer versions are breaking our static checks.
types-docutils==0.17.5
types-toml
extras =
mypy

commands =
mypy \
Expand Down
Loading