[SERVER-68566] Disable pylinters on buildscripts/gdb/ Created: 04/Aug/22  Updated: 29/Oct/23  Resolved: 05/Aug/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 6.1.0-rc0

Type: Improvement Priority: Major - P3
Reporter: Daniel Gottlieb (Inactive) Assignee: Daniel Gottlieb (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-69148 Revert SERVER-68566 Closed
Related
is related to SERVER-68593 Disable the following python warnings Closed
is related to SERVER-68594 Add vscode format/lint on save Closed
is related to SERVER-68455 Clean up and publish GDB helpers for ... Closed
Backwards Compatibility: Fully Compatible
Participants:

 Description   

Many of the linters are low-value, such as the docstring requirement. It's almost universally elided with a docstring that mimics the function name, e.g:

    def children(self):
        """Children."""

I think we can consider turning some* pylinters on for our gdb scripts, but right now we're creating a barrier to entry on a part of the codebase where we should be sharing more tools rather than using a poor metric for code quality that has veto rights.

Most scripts we want to add are already being shared, but outside of the repository. The utility is apparent.



 Comments   
Comment by Daniel Gottlieb (Inactive) [ 05/Aug/22 ]

I've closed this ticket, but feel free to re-open if you just want to piggy-back on it for re-adding the better curated rules.

Comment by Daniel Gottlieb (Inactive) [ 05/Aug/22 ]

Yes, I'm aware. That's what I use today.

But that's not good enough UX. Today many engineers are using some version of "format my code on save such that lint checks will pass". For server engineers working only on c++ and js files, that's easy (pseudocode-y):

def format_my_code_on_save(filename):
  exec("clang-format -i %s".format(filename))

But for engineers that may touch python files under the jurisdiction of pylint, we now need:

def format_my_code_on_save(filename):
  if filename.endswith(".py"):
    exec("pylinters fix...")
  elif filename.endswith(".cpp") ||
    filename.endswith(".h") ||
....
    exec("clang-format...")

If that logic to determine which file maps to which "formatter" were baked into some new published shell script, engineers could just have:

def format_my_code_on_save(filename):
  exec("buildscripts/fix_my_stuff.sh %s".format(filename))

Modulo some portability improvements.

Comment by Ryan Egesdahl (Inactive) [ 05/Aug/22 ]

daniel.gottlieb@mongodb.com Such a thing does exist already in the repo as buildscripts/pylinters.py lint-all, and you can use fix to automatically fix them if you want. The commit queue uses lint-patch.

Comment by Daniel Gottlieb (Inactive) [ 05/Aug/22 ]

Using the following grep as a seed of warnings that might be worth disabling:

grep -r 'pylint:' buildscripts/ | grep -v enable

I've written a script to parse/count them up:

import re
 
mp = {}
q = re.compile('disable=(.*)')
for line in open('disabled_pylint.txt'):
    w = q.findall(line)[0].split(',')
    for k in w:
        if k in mp:
            mp[k] += 1
        else:
            mp[k] = 1
 
lst = []
for k,v in mp.items():
    lst.append((k, v))
 
lst.sort(key=lambda x: x[-1], reverse=True)
from pprint import pprint
pprint(lst)

I don't know if that passes our pylint, but in the spirit of investigation, I've written that to be intentionally readable unfriendly

The results:

[('invalid-name', 150),
 ('too-many-arguments', 104),
 ('missing-docstring', 70),
 ('no-self-use', 63),
 ('protected-access', 62),
 ('too-many-instance-attributes', 61),
 ('too-many-branches', 52),
 ('unused-argument', 47),
 ('too-many-locals', 39),
 ('wrong-import-position', 33),
 ('too-many-statements', 25),
 ('broad-except', 23),
 ('no-value-for-parameter', 23),
 ('undefined-variable', 21),
 ('bare-except', 17),
 ('too-many-lines', 16),
 ('too-many-public-methods', 11),
 ('too-many-return-statements', 10),
 ('global-statement', 10),
 ('no-method-argument', 9),
 ('super-init-not-called', 8),
 ('arguments-differ', 7),
 ('abstract-method', 7),
 ('too-many-function-args', 6),
 ('non-parent-init-called', 6),
 ('no-name-in-module', 5),
 ('invalid-metaclass', 5),
 ('inconsistent-return-statements', 4),
 ('R0201', 4),
 ('W0703', 4),
 ('unused-variable', 3),
 ('unsubscriptable-object', 3),
 ('cell-var-from-loop', 3),
 ('missing-class-docstring', 2),
 ('missing-function-docstring', 2),
 ('invalid-str-returned', 2),
 ('R0915', 2),
 ('C0103', 2),
 (' too-many-locals', 2),
 ('invalid-unary-operand-type', 2),
 ('C0413', 1),
 ('pointless-statement', 1),
 ('C0111', 1),
 (' redefined-builtin', 1),
 (' redefined-outer-name', 1),
 ('stop-iteration-return', 1),
 ('wildcard-import', 1),
 ('eval-used', 1),
 ('len-as-condition', 1),
 ('ungrouped-imports', 1),
 ('method-hidden', 1),
 ('expression-not-assigned', 1),
 ('wrong-import-order', 1),
 ('not-an-iterable', 1),
 ('chained-comparison', 1),
 ('consider-using-ternary', 1),
 ('too-many-nested-blocks', 1),
 ('undefined-all-variable', 1),
 (' too-many-instance-attributes', 1),
 (' too-many-statements', 1),
 ('too-many-format-args', 1),
 ('keyword-arg-before-vararg', 1),
 ('raising-bad-type', 1),
 (' invalid-metaclass', 1),
 ('R0912', 1),
 ('R0914', 1),
 (' too-many-public-methods', 1),
 ('too-many-boolean-expressions', 1),
 ('attribute-defined-outside-init', 1),
 ('unused-import', 1),
 ('unsupported-membership-test', 1),
 ('', 1),
 ('import-outside-toplevel', 1),
 ('too-many-ancestors', 1),
 ('bad-mcs-classmethod-argument', 1),
 ('bad-classmethod-argument', 1),
 ('c-extension-no-member', 1)]

I'm going to blindly recommend everything >= 10 also be suppressed.

We are also going to create and/or provide a way for running python formatting/linting as a script/pluggable into vs code

To be clear, I think the highest value thing is a single script that can correctly identify and run the proper in-place format tool for any file in the mongo repository that is linted against. Today we use clang-format works for our c++ and js code with a single invocation. But fixing python files in-place requires a separate invocation.

Comment by Alex Neben [ 05/Aug/22 ]

From our discussion we are going to disable the following python warnings
1. pydocstring (no forcing docstring comments on each function)
2. function argument limiting (functions can have as many arguments as your heart desires )

We are also going to create and/or provide a way for running python formatting/linting as a script/pluggable into vs code

Generated at Thu Feb 08 06:11:09 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.