launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+416744@xxxxxxxxxxxxxxxxxx>]

@guruprasad: I added a couple of comments why I think the documentation needed some updates. What do you think?

Diff comments:

> diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst
> index e4c23d1..64753c6 100644
> --- a/CONTRIBUTING.rst
> +++ b/CONTRIBUTING.rst
> @@ -6,8 +6,6 @@ Prerequisites
>  
>  * Python 3.8+
>  * `tox <https://tox.wiki/en/latest/>`_
> -* `pyenv <https://github.com/pyenv/pyenv>`_, if you want to use and test against
> -  multiple versions of Python

I removed pyenv for several reasons:
It is not really a requirement; you can use your os' installer (at least on Fedora), you can use deadsnake's ppa on Ubuntu (which I did), you can compile Python, ...

And I have never seen a Python project where the developer documentation explains how to install Python. I think we can assume a basic knowledge when somebody wants to contribute to this project.

There is one counter example. buildout ships a Makefile, which installs all supported Python versions... which almost scared me away from contributing... :-)

>  
>  Usage
>  -----
> @@ -23,20 +21,23 @@ List the ``tox`` environments available for this project.
>  
>    .. code:: bash
>  
> -    $ tox -l
> -    lint
> -    mypy
> -    py38
> -    py39
> -    py310
> +    $ tox -lv
> +    default environments:
> +    lint     -> run linters
> +    mypy     -> run static type checker
> +    py38     -> run test suite
> +    py39     -> run test suite
> +    py310    -> run test suite
> +    coverage -> generate coverage report

I added the missing descriptions to the `tox.ini` file.

>  
>  Run the project's tests.
>  
>    .. code:: bash
>  
> -    $ tox -e py38  #  Replace with the installed Python version, if 3.8 is unavailable.
> +    $ tox -e py38  #  You can replace ``py38`` with another Python version.
>  
> -Since ``tox`` uses ``pytest`` under the hood to run the tests, arguments can be passed to pytest.
> +Since ``tox`` uses `pytest <https://docs.pytest.org/>`_ under the hood to run
> +the tests, arguments can be passed to ``pytest``.

I think it is never a bad idea to link to a project's documentation.

>  
>    .. code:: bash
>  
> @@ -50,14 +51,15 @@ Run the tests with coverage.
>  
>      $ tox -e coverage
>  
> -Run the linter.
> +Run the linters.

We have more than one.

>  
>    .. code:: bash
>  
>      $ tox -e lint
>  
> -Alternatively, you can run ``pre-commit install`` to install the git pre-commit hooks,
> -which run the linter.
> +We also support running linters via `pre-commit <https://pre-commit.com/>`_.
> +If you want ``pre-commit`` to run automatically on ``git commit``,
> +you need to run ``pre-commit install`` once.
>  
>  Run the ``mypy`` static type checker.
>  
> @@ -72,12 +74,18 @@ Update the requirements and regenerate ``requirements.txt``.
>      $ <modify requirements.in>
>      $ tox -e pip-compile
>  
> -If any of the ``tox`` environments use a version of Python that is not installed, edit
> -``tox.ini`` and replace the value for the ``basepython`` key under that environment.

Actually, we have a configuration file so all developers use the same configuration.

Manipulating configuration files before committing changes is pretty dangerous.

I am pretty sure that e.g. black acts slightly different for different Python versions, possibly also other tools.

While we have no reproducible builds, we intentionally pin linter versions in our pre-commit configuration, and the Python versions in tox.ini, at least to come close.

I won't spend time to verify, but possibly the changed Python version was the issue why we spent about 30 minutes to figure out why the linters worked differently on your pc compared to mine.

I remember debugging a tox issue which only showed on Python 3.5, not on 3.5.1 :-) While we can't be that religious, let's use the same major versions at least.

And for the record - it does not matter whether we use Python 3.8 for the linters or Python 3.9 - but we need to stay at one version.

That is a different story for building the docs. The docs on Read the Docs are built with Python 3.9, so we really also want to do this locally, in order to avoid possibly subtle but hard to debug issues.

And yes, here the documentation lacked the information. I added that to the tox environment.

> +Build the documentation locally.
>  
> -To update the `project's documentation
> -<https://lpcraft.readthedocs.io/en/latest/>`_, you need to trigger a manual
> -build on the project's dashboard on https://readthedocs.org.
> +  .. code:: bash
> +
> +    $ tox -e docs

As we decided to list all tox environments, let's stay consistent and add the documentation generation, too.

> +
> +.. note::
> +
> +    In order to update the `project's documentation
> +    <https://lpcraft.readthedocs.io/en/latest/>`_ online,
> +    after having pushed your changes to the repository, you need to trigger a
> +    manual build on the project's dashboard on https://readthedocs.org.
>  
>  Getting help
>  ------------
> diff --git a/tox.ini b/tox.ini
> index 318e847..cba2529 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -10,6 +10,8 @@ skip_missing_interpreters =
>      true
>  
>  [testenv]
> +description =
> +    run test suite

I added the missing description for all tox environments - not sure why I forgot to add them in the first place.

>  commands =
>      pytest {posargs}
>  deps =
> @@ -56,6 +66,9 @@ commands =
>      coverage report -m  --fail-under=100
>  
>  [testenv:docs]
> +description =
> +    generate documentation
> +# the Python version here matches the one in .readthedocs.yaml

I added the missing information why we need to use Python 3.9 here.

Usually, I do not add comments to tox.ini, as the tox.ini formatter strips them - but as we do not use it, we are ok with comments :-)

>  basepython = python3.9
>  extras = docs
>  commands =


-- 
https://code.launchpad.net/~jugmac00/lpcraft/+git/lpcraft/+merge/416744
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/lpcraft:improve-contribution-documentation into lpcraft:main.