Improve instructions for new contributors #1394

FlorentJeannot · 2021-04-27T21:40:28Z

When I made my first PR, I was expecting tox to be in testing, because the doc says:

- Install pip-tools in development mode and its test dependencies with pip install -e .[testing].
- Check with `tox -e checkqa` to see your changes are not breaking the style conventions.

Changelog-friendly one-liner: Improve instructions for new contributors

Contributor checklist

Provided the tests for the changes.
Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

setup.cfg

webknjaz

Tox is a central piece of automation but it is not a test dependency. It manages envs and their deps. Instead, the instructions should be updated to use tox. One can use --devenv if a regular virtualenv is needed.

FlorentJeannot · 2021-04-28T22:28:20Z

Thanks for your feedback, I made the changes. Let me know if you think we should tweak the instructions.

CONTRIBUTING.md

webknjaz · 2021-04-28T22:43:49Z

CONTRIBUTING.md

 - Check with `tox -e checkqa` to see your changes are not breaking the style conventions.
- Always provide tests for your changes.
+- Always provide tests for your changes, use `tox -e coverage` to run the tests with coverage reports.


Honestly, running tox will run all the test envs with coverage + linters. Not sure if mentioning just coverage is needed. Besides, codecov will report back to PRs if it's decreased, I think. Maybe this sort of detail belongs to a development guide rather than contribution instructions. I see such details as non-mandatory things that may help during the development process but should be used if the developer actually needs them. Same with use tox -e py to run on test env with the current Python, use tox -e pyXY to specify other Python, and so on — we could basically end up documenting how tox works but that would probably be too overwhelming.
Mentioning tox -av along the lines might be useful, OTOH.

tox -e checkqa was already there when I started contributing to pip-tools, so it already looked like a mix of contribution and development guide to me.
We could eventually regroup these instructions in a Development guide section of this file? I think making another file for this is too much. I personally like a README that tells me exactly what to do to get started quickly. Later I can go to the tool's website to understand it better (we could provide the links too).

Also, when I ran tox -e coverage and tox -e checkqa, it ran the checks and the tests only for my version of python. Do you suggest that we mention to run tox only, to run everything once?

By the way what I did for my first PR:

Ensured the tests were passing for my python version, checked the code coverage reports about my new tests

Run the code linters which fixed my files

See in my PR that the CI succeeded for every python versions

Maybe tox -p all?

FYI tox -p all is really slow on my old macbook air:

✔ OK py39-piplatest-coverage in 9 minutes, 36.482 seconds ✔ OK py39-pipprevious-coverage in 10 minutes, 20.35 seconds ✔ OK py39-pip20.3-coverage in 10 minutes, 20.76 seconds ✔ OK py39-pipmain-coverage in 10 minutes, 40.551 seconds

So do you guys consider running tox with all test environments mandatory?
Also it seems to run only for py39, the current version I am using. Does that mean devs should install other python versions on their machine so tox can use them as well? If so, this is something we should mention I think? (it's my first time contributing to an open source project and I am not used to how tox works)

According to what you say, I suggest to change Getting started to Development Guidelines.
Inside, we could suggest command lines to get things done correctly. But it should be kept simple, as @webknjaz said, we don't want to write a full doc about the tools and all possibilities.

Inside the contributing guidelines, we could have:

- Always provide tests for your changes. - Check with `tox` that your changes are not breaking the style conventions.

And inside Development Guidelines, we could have what we wrote in Getting started and add a line or two about testing the branch, probably $ tox -p all.

What do you think?

webknjaz · 2021-04-28T22:45:01Z

@FlorentJeannot this PR probably deserves a title change now that it doesn't do what the said title says 🤷‍♂️

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

for more information, see https://pre-commit.ci

FlorentJeannot · 2021-04-28T23:38:34Z

@FlorentJeannot this PR probably deserves a title change now that it doesn't do what the said title says 🤷‍♂️

Agreed, I updated the PR's title. However I'm not sure I can update the branch name without any trouble. If you know how to please let me know.
Thanks!

for more information, see https://pre-commit.ci

atugushev · 2021-04-29T03:53:53Z

CONTRIBUTING.md

 ## Project Contribution Guidelines

 Here are a few additional or emphasized guidelines to follow when contributing to pip-tools:

- Install pip-tools in development mode and its test dependencies with `pip install -e .[testing]`.
 - Check with `tox -e checkqa` to see your changes are not breaking the style conventions.


We could drop this line since pre-commit.ci checks and autofixes QA issues. What do you think?

Yes that's a nice catch.
Also if we suggest to run tox -p all then I think it will still be run (which is not a bad thing anyway)?

I think it's a good idea to have -p there. The ultimate goal is to let people know how to run the same test matrix that the CI runs locally. And running linters locally can also be useful because people often want to reproduce the offenses in their env to be able to address them. Rerunning the whole CI on every typo fix push is wasteful.

CONTRIBUTING.md

…idelines. Tweaked "Project Contribution Guidelines".

…ox-in-setup-cfg

for more information, see https://pre-commit.ci

FlorentJeannot · 2021-05-02T21:38:24Z

@webknjaz @atugushev I updated the contribution guidelines. I think it's better to tell users to install everything inside the project's virtualenv. I ran the command lines to double-check that it works. Let me know what you think.

Also, if you could take some time to review #1392 that would be kind of you. I described the issue as much as I could to help you understand it. Thanks in advance!

webknjaz · 2021-05-02T22:16:01Z

The whole workflow is tox-centric, as almost any Python project of this sort today. Throwing that away and maintaining a copy of instructions that tox has embedded keeping it in sync all the time is a thankless job.
This is why I'm huge 👎 on removing it from the guide. The whole idea of tox is that it provides the same experience in the CI and locally.

webknjaz · 2021-05-02T22:18:11Z

By the way, instead of guessing how to escape the brackets for zsh, it's easier to just wrap that with single quotes.

webknjaz · 2021-05-02T22:22:28Z

The typical experience is to just do tox -e py -- <extra args for pytest> without having a raw venv for testing. That venv can be useful for developing, although I just reuse what tox already has — .tox/py/bin/python gets me in just as easily.

webknjaz · 2021-05-02T22:26:36Z

We should not maintain a usage guide on every possible way to set up the env/python/etc — there are great writeups describing a million ways to do this already, we could just point at https://packaging.python.org and https://realpython.com instead.

FlorentJeannot · 2021-05-02T22:28:44Z

We should not maintain a usage guide on every possible way to set up the env/python/etc — there are great writeups describing a million ways to do this already, we could just point at https://packaging.python.org and https://realpython.com instead.

Ok let's do that instead.

…ox-in-setup-cfg # Conflicts: # CONTRIBUTING.md

CONTRIBUTING.md

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

atugushev

Looks great! Thanks @FlorentJeannot for working on this patiently and @webknjaz for reviews!

FlorentJeannot · 2021-05-06T06:57:51Z

@atugushev No problem! Happy to contribute to this project!
Thanks guys for your reviews and your help!

add tox in setup.cfg

9768ef0

atugushev reviewed Apr 28, 2021

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

webknjaz requested changes Apr 28, 2021

View reviewed changes

reverted changes in setup.cfg and improved CONTRIBUTING.md

d44763e

tweaked word in CONTRIBUTING.md

1c83b28

webknjaz reviewed Apr 28, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

webknjaz reviewed Apr 28, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

webknjaz reviewed Apr 28, 2021

View reviewed changes

webknjaz requested review from webknjaz and atugushev April 28, 2021 22:44

FlorentJeannot and others added 3 commits April 29, 2021 00:58

Update CONTRIBUTING.md

2ffc06d

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Update CONTRIBUTING.md

ec10aec

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

[pre-commit.ci] auto fixes from pre-commit.com hooks

13581ef

for more information, see https://pre-commit.ci

FlorentJeannot changed the title ~~add tox in setup.cfg~~ Improve instructions for new contributors Apr 28, 2021

FlorentJeannot and others added 4 commits April 29, 2021 01:46

tweaked markdown format in getting started

cfaa767

[pre-commit.ci] auto fixes from pre-commit.com hooks

f491b6e

for more information, see https://pre-commit.ci

removed markdown added by pre-commit-ci bot

42dc85c

[pre-commit.ci] auto fixes from pre-commit.com hooks

16e5118

for more information, see https://pre-commit.ci

atugushev reviewed Apr 29, 2021

View reviewed changes

webknjaz reviewed May 1, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

FlorentJeannot and others added 3 commits May 2, 2021 23:29

Moved "Getting Started" to "Development Guidelines" and added more gu…

5e9be24

…idelines. Tweaked "Project Contribution Guidelines".

Merge remote-tracking branch 'origin/add-tox-in-setup-cfg' into add-t…

86b498d

…ox-in-setup-cfg

[pre-commit.ci] auto fixes from pre-commit.com hooks

146ca44

for more information, see https://pre-commit.ci

FlorentJeannot added 3 commits May 3, 2021 00:47

revert guideline, suggest websites to get started

76f3cc8

Merge remote-tracking branch 'origin/add-tox-in-setup-cfg' into add-t…

fe0a163

…ox-in-setup-cfg # Conflicts: # CONTRIBUTING.md

add missing line in guideline

dde55b9

webknjaz reviewed May 2, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

webknjaz reviewed May 2, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

FlorentJeannot added 3 commits May 3, 2021 21:04

tweak guideline to make the flow tox-centric

1ce92b4

tweak sentence

1ce0e8c

add missing dot

1b549fb

webknjaz reviewed May 5, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

webknjaz reviewed May 5, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

webknjaz requested a review from atugushev May 5, 2021 19:12

FlorentJeannot and others added 2 commits May 5, 2021 21:22

Update CONTRIBUTING.md

108cb54

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Update CONTRIBUTING.md

e50a524

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

atugushev added the docs Documentation related label May 6, 2021

This comment has been minimized.

Sign in to view

atugushev approved these changes May 6, 2021

View reviewed changes

atugushev added this to the 6.2.0 milestone May 6, 2021

webknjaz approved these changes May 6, 2021

View reviewed changes

webknjaz merged commit 26ee416 into jazzband:master May 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve instructions for new contributors #1394

Improve instructions for new contributors #1394

FlorentJeannot commented Apr 27, 2021 •

edited by atugushev

Loading

webknjaz left a comment

FlorentJeannot commented Apr 28, 2021

webknjaz Apr 28, 2021

FlorentJeannot Apr 28, 2021

atugushev Apr 29, 2021

FlorentJeannot Apr 29, 2021

webknjaz commented Apr 28, 2021

FlorentJeannot commented Apr 28, 2021

atugushev Apr 29, 2021

FlorentJeannot Apr 29, 2021 •

edited

Loading

webknjaz May 2, 2021

FlorentJeannot commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

FlorentJeannot commented May 2, 2021

This comment has been minimized.

atugushev left a comment

FlorentJeannot commented May 6, 2021

Improve instructions for new contributors #1394

Improve instructions for new contributors #1394

Conversation

FlorentJeannot commented Apr 27, 2021 • edited by atugushev Loading

Contributor checklist

webknjaz left a comment

Choose a reason for hiding this comment

FlorentJeannot commented Apr 28, 2021

webknjaz Apr 28, 2021

Choose a reason for hiding this comment

FlorentJeannot Apr 28, 2021

Choose a reason for hiding this comment

atugushev Apr 29, 2021

Choose a reason for hiding this comment

FlorentJeannot Apr 29, 2021

Choose a reason for hiding this comment

webknjaz commented Apr 28, 2021

FlorentJeannot commented Apr 28, 2021

atugushev Apr 29, 2021

Choose a reason for hiding this comment

FlorentJeannot Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

webknjaz May 2, 2021

Choose a reason for hiding this comment

FlorentJeannot commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

webknjaz commented May 2, 2021

FlorentJeannot commented May 2, 2021

This comment has been minimized.

atugushev left a comment

Choose a reason for hiding this comment

FlorentJeannot commented May 6, 2021

FlorentJeannot commented Apr 27, 2021 •

edited by atugushev

Loading

FlorentJeannot Apr 29, 2021 •

edited

Loading