openerp-community team mailing list archive

[Sandy Carter <sandy.carter@xxxxxxxxxxxxxxxxxxxx>]

Hello community,

I am writing for two reasons. One is to inform you that there has been a
rather drastic change to what Travis tests in code quality. Second is a
proposal for a more sensible approach to changes to automatic tests.

====

Firstly, there are new checks done by pylint [1]:
(all these can be seen with pylint2 --help-msg <msg-id>[,<msg-id>])

* W0104 pointless-statement: Used when a statement doesn't have (or at
least seems to) any effect.
* W0105 pointless-string-statement: Used when a string is used as a
statement (which of course has no effect).
* W0109 duplicate-key: Used when a dictionary expression binds the same
key multiple times.
* W0403 relative-import: Used when an import relative to the package
directory is detected.
* W0404 reimported: Used when a module is reimported multiple times.
* W0621 redefined-outer-name: Used when a variable's name hide a name
defined in the outer scope.
* W0622 redefined-builtin: Used when a variable or function override a
built-in.

This adds on to the current tests:

* E0101 return-in-init: Used when the special class method __init__ has
an explicit return value.
* E1124 redundant-keyword-arg: Used when a function call would result in
assigning multiple values to a function parameter, one value from a
positional argument and one from a keyword argument.
* E1306 too-few-format-args: Used when a format string that uses unnamed
conversion specifiers is given too few arguments
* I0013 file-ignored: Used to inform that the file will not be checked
* W0101 unreachable: Used when there is some code behind a "return" or
"raise" statement, which will never be accessed.
* W0102 dangerous-default-value: Used when a mutable value as list or
dictionary is detected in a default value for an argument.
* W1111 assignment-from-none: Used when an assignment is done on a
function call but the inferred function returns nothing but None.

And Odoo specific tests (called odoo-lint):
* EO001 print statement used: Used when there is a call to print

The rules for flake8 are:
* max-line-length = 79
* ignore unused imports in __init__.py

I don't think anyone has gone out and documented this yet, and this is
just a quick glance at the confs on my part.

These rules should be put somewhere, I suggest the wiki of
oca/maintainer-quality-tools.

====

Secondly, the approach to changing these checks needs to be revised.
Here is my proposal:

We created an organization called oca-travis[2] with this org,
pro-active fixes to repos can be done without affecting ongoing projects
on oca before merging the new changes to the rules.

1. Before adding new rules, make sure a majority of projects are green
(see the controlboard
https://github.com/OCA/maintainer-quality-tools/pull/48). If they
aren't, an effort should be done to get these tests to work with the
current rules.
2. Make sure the community is informed about the new rules. I am talking
an email not an obscure PR or Issue that only the most hardcore
reviewers will see.
3. Make a PR
4. Accept PR with 3 people, do not merge yet.
5. Give a deadline before the new changes take effect, so that repos can
be ready
6. The ones proposing a new rule, _must_ participate in fixing the
errors _pro-actively_(ie. code sprint), a script to fork all repos in
the org oca-travis and test them using a the MQT from PRs should not be
very complicated to write.
7. Close to the deadline, audit the repos on oca-travis and see if the
deadline should be extended.
8. Remind the community
9. Merge PR on MQT and pylint fixes from oca-travis
10. Update the wiki to document these new rules.

At this point when the community complain, point to the emails and show
that procedure was followed.

Is this sensible?

[1] https://github.com/OCA/maintainer-quality-tools/pull/127
[1] https://github.com/orgs/oca-travis

Attachment: signature.asc
Description: OpenPGP digital signature