#1563 Refresh the contributor documentation
Closed: Insufficient data a year ago by wombelix. Opened 7 years ago by jcline.

The contributor documentation is somewhat outdated and needs some general freshening up. Beyond that, though, I think it would be good to decide upon some standards that must be met by all pull requests before they are accepted, document those as part of the contributor guide, and enforce them strictly.

Here are my proposals for coding standards that we uphold:

  • The test suite must pass
  • Bug fixes must (unless it makes no sense to do so) include one or more tests to demonstrate the issue is fixed. These tests should fail before the bug fix is applied, and pass afterwards.
  • All new code must include complete test coverage.
  • All new code must be PEP8-compliant and this must be enforced by the test suite.

  • The test suite must pass

Most definitely agreed and I think we already have this

  • Bug fixes must (unless it makes no sense to do so) include one or more tests to demonstrate the issue is fixed. These tests should fail before the bug fix is applied, and pass afterwards.
  • All new code must include complete test coverage.

For those two, I think I would prefer a "should", making it easier for people to contribute, even if that means we end up adding the tests afterward, and even for us it is sometime convenient to be able to fix the bug first and spend time on the tests after.

  • All new code must be PEP8-compliant and this must be enforced by the test suite.

In theory I agree with you, but I recently got an example of a change (#1382) that was made because pep8.py complained (on F24 and not on F23) and where I preferred the old way (which turned out to have been adjusted in pep-0008). So I wonder if I would not rather error on the side of human review, eventually doing a pep8 global fix PR ones in a while.

Bug fixes must (unless it makes no sense to do so) include one or more tests to demonstrate the issue is fixed. These tests should fail before the bug fix is applied, and pass afterwards.
All new code must include complete test coverage.

For those two, I think I would prefer a "should", making it easier for people to contribute, even if that means we end up adding the tests afterward, and even for us it is sometime convenient to be able to fix the bug first and spend time on the tests after.

I'm all for making it easy to contribute. If it's a new contributor, I typically like to offer to write the tests for them if they don't have the time/inclination/understanding to do it themselves. I just take their PR, add a commit on top of it, and then that's reviewed/merged. If we adopt that strategy we can still hold ourselves to the 100% new code coverage requirement without discouraging new contributors. Does that sound reasonable?

All new code must be PEP8-compliant and this must be enforced by the test suite.

In theory I agree with you, but I recently got an example of a change #1382) that was made because pep8.py complained (on F24 and not on F23) and where I preferred the old way (which turned out to have been adjusted in pep-0008). So I wonder if I would not rather error on the side of human review, eventually doing a pep8 global fix PR ones in a while.

I'm not terribly familiar with pep8.py. I've always used flake8 and it lets you specify exceptions to specific PEP8 rules. We can just configure the linter with those exceptions. My view on style has always been "pick an automated tool and configure it; if it doesn't complain, I won't either". This makes contributions easier since there's no longer any guesswork about personal preferences. If the linter approves, you're good to go.

Bug fixes must (unless it makes no sense to do so) include one or more tests to demonstrate the issue is fixed. These tests should fail before the bug fix is applied, and pass afterwards.
All new code must include complete test coverage.

For those two, I think I would prefer a "should", making it easier for people to contribute, even if that means we end up adding the tests afterward, and even for us it is sometime convenient to be able to fix the bug first and spend time on the tests after.

I'm all for making it easy to contribute. If it's a new contributor, I typically like to offer to write the tests for them if they don't have the time/inclination/understanding to do it themselves. I just take their PR, add a commit on top of it, and then that's reviewed/merged. If we adopt that strategy we can still hold ourselves to the 100% new code coverage requirement without discouraging new contributors. Does that sound reasonable?

Pagure doesn't have that feature, you would have to cancel/close their PR and
open a new one with their commits as well as yours.

I do have in mind something around PR collaboration though, which would allow
this scenario.

All new code must be PEP8-compliant and this must be enforced by the test suite.

In theory I agree with you, but I recently got an example of a change #1382) that was made because pep8.py complained (on F24 and not on F23) and where I preferred the old way (which turned out to have been adjusted in pep-0008). So I wonder if I would not rather error on the side of human review, eventually doing a pep8 global fix PR ones in a while.

I'm not terribly familiar with pep8.py. I've always used flake8 and it lets you specify exceptions to specific PEP8 rules. We can just configure the linter with those exceptions. My view on style has always been "pick an automated tool and configure it; if it doesn't complain, I won't either". This makes contributions easier since there's no longer any guesswork about personal preferences. If the linter approves, you're good to go.

I see what you mean but my experience so far as been mostly with using pep8.py
which has sometime from version to version introduced new checks that I did not
agree with.
Maybe I should give flake8 a try.

Do you think we could make flake8 spew warnings instead of errors when running
the in test suite, maybe as a first step before enforcing something?

The last update was 6 years ago, no further requests, updates or actionable tasks since then, I'm going to close this issue for now to reduce our backlog.

Metadata Update from @wombelix:
- Issue close_status updated to: Insufficient data
- Issue status updated to: Closed (was: Open)

a year ago

Login to comment on this ticket.

Metadata