openerp-community team mailing list archive

[Georges Racinet <gracinet@xxxxxxxxx>]

Hi Sandy,

On 11/13/2014 06:13 PM, Sandy Carter wrote:
> Following my proposed procedure, I am informing the community that we
> intend to add a new style restriction [1].
>
>
> W0402 deprecated-module:Used a module marked as deprecated is imported.
>
> Modules marked as deprecated: pdb,pudb,ipdb
>
> The reason behind this is that these are developement modules used to
> pause code in the middle of execution in order to debug. They are not
> intended for production but can be forgotten in the code and commited.
> To catch these is necessary since it halt code execution and this is not
> desired in production.
>
> The implication of this is that any code with any of these modules will
> fail on travis and be rejected by the reviewer.
do you forbid pdb.set_trace() or the import of pdb and friends as a whole ?
If that's the latter, I'd be firmly against it, since it also forbids
very useful stuff, such as insertion of (optional, of course) pdb
post_mortem calls.
Of course if there's a way to bypass it with a # noqa comment or
equivalent, this objection does not stand.


>
> We await your feedback. If there are no objections, this new rule will
> be added to the entirety of OCA repos and any modules using them will
> fail the project as a whole and as a result, potentially block all pull
> requests until these modules are fixed.
>
> [1] https://github.com/OCA/maintainer-quality-tools/pull/113
>
> Le 2014-11-13 11:42, Sandy Carter a écrit :
>> 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
>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openerp-community
>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~openerp-community
>> More help   : https://help.launchpad.net/ListHelp
>>
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp