PyLint integration to runbot

[xmo-odoo]

Runs pylint embedded, filters lint messages to only keep those applying to lines touched by the pull request and, the messages as inline comments on the github PR and sets a cl/lint status, as visible on odoo/odoo#10374. @tivisse can confirm that it's super extra awesome.

Currently adds dependencies on pylint for the linting itself and edx-lint for a few interesting lints by @nedbat (which we used with @mart-e to find and fix non-static strings used in translation functions). Works on both "primary" repository (odoo/odoo) and "modules" repositories (with all modules at the repository root).

Advantages:

• runs lint at all
• fast, only passes the changed files to pylint then filters to only process the messages on changed lines, checking most PRs takes a few seconds locally, the slowest part is posting the review comments (no bulk API, and we may need to keep a requests session around for connection pooling)

This is a working technical basis, but not exactly done:

• [x] not sure how linting should be configured, currently it's a hardcoded list of lint enabled lint messages, should linting be configured per build? branch? repo? Via a configuration file in the repo? None and the current solution is fine?
• [x] running it multiple times on the same branch will result in repeated messages, getting existing comments and updating/replacing/replying to them might be a good idea?
• [x] all linting messages are currently hard errors (failures), that shouldn't be a huge issue as they're relatively conservative and they can be disabled using pylint annotations (#pylint: disable=w1, w2). Still if we start adding custom lints maybe there should be a way for lints to be configured as "mark for extra review", and be reviewable? Not sure how.
• [x] __openerp__.py will systematically trigger "statement seems to have no effect", not sure how to ignore only this specific lint on this specific file aside from special-casing it in the already overridden is_message_enabled. We may want to add manifest lints in the future so disabling it entirely may not be such a good idea hardcoded a special case
• [ ] plenty of custom lints to potentially add:
  • [ ] OCA has a pylint-odoo plugin with a few interesting ones
  • [x] leftover print and assert (windows builds use "optimised" bytecode so asserts are stripped)
  • [x] leftover debugger statements (probably just calls to .set_trace()?)
  • [x] translations missing from POT files
  • [x] flagging non-literal SQL queries for review
  • [x] incorrect super/with_context calls (odoo/odoo#5738)
  • [x] check for dict(context, key=v) where context may still be None. (odoo/odoo#10679) requires significant control flow analysis which Pylint doesn't seem to provide for
  • [x] check for dict.update(literal_dict)
  • [ ] other ideas?
• [x] the changes to github interfacing should be improved, it's currently a "works on my machine" change to get the diff-fetching working on private repos
• [x] review-comments currently generate one email alert each (from github) (may be due to not reusing sessions) changed to set runbot status to warn, log the issues to the runbot's log and not post messages to the PR itself anymore. Still sets a failure ci/lint status on the PR.

/cc @odony @mart-e @rco-odoo @antonylesuisse @dbo-odoo

[odony]

For the record, the OCA also has https://github.com/OCA/runbot-addons/tree/9.0/runbot_pylint - do you know how this one differs?

[xmo-odoo]

@odony I hadn't seen it, so I have no idea how they differ. Quickly going over the readme (so I may well be wrong and miss things)

• OCA's is configurable (at all) by a repo to a "pylint configuration" repo
• it's integrated with the runbot matrix view (in that it will trigger yellow warning cells, and expose the pylint log) but not github PRs
• it runs on the whole target repository? Really not sure about that one
• it also seems to be a more classical shelled-out pylint invocation

[tivisse]

I cannot wait to see the Github Integration into mail. So that I can see all these wonderful messages in the chatter. @mart-e #puke

[xmo-odoo]

@odony @antonylesuisse @rco-odoo barring the necessity of additional custom lints (sadly the detection of methods using None context didn't really work out, no way to easily do the CFA) I think it's in a pretty good shape, at least for an initial deployment.

[antonylesuisse]

Can we minize the change in runbot ? Minimum number of diff in runbot,py ? Orthogonal status for lint only in the pylint module.

[xmo-odoo]

Can we minize the change in runbot ? Minimum number of diff in runbot,py ?

If a bunch of stuff being duplicated in the pylint extension doesn't matter then sure I can revert pretty much all the changes in runbot (and move that stuff to just runbot_pylint)

Orthogonal status for lint only in the pylint module.

As in, add a second status field to builds then go and "extend" (copy/paste a bunch of) the HTML views to include that new status in the display?

[xmo-odoo]

Modified reporting to add an icon to PR branches for indication of linting result, and reverted changes to per-build state (since the linting is done at a commit, but it's really done on the revision range involved in a PR, it's not done on a specific commit)