Skip to content

Bump Pylint to 2.17 and flake8 to 6.0 #359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bebound merged 30 commits into from
Jul 14, 2023
Merged

Bump Pylint to 2.17 and flake8 to 6.0 #359

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ac7c673
Update
bebound Feb 6, 2023
44c36ee
Minor fix
bebound Feb 6, 2023
d4070e8
Enable no_self_use
bebound Feb 6, 2023
1928f29
Minor fix
bebound Feb 6, 2023
c60042d
Minor fix
bebound Feb 6, 2023
3a22e88
Minor fix
bebound Feb 6, 2023
87cb41c
Minor fix
bebound Feb 6, 2023
2fd99af
Fix flake8
bebound Feb 6, 2023
998a0bc
Update setup.py
dciborow Mar 10, 2023
4cd84e7
Update cli_pylintrc
dciborow Mar 10, 2023
7a2c4fe
Update ext_pylintrc
dciborow Mar 10, 2023
a4bb458
Merge branch 'dev' into dciborow/pylint
dciborow Mar 10, 2023
0fa543d
Merge branch 'dev' into dciborow/pylint
dciborow May 23, 2023
a071b95
Update .pylintrc
dciborow May 24, 2023
1028936
Apply suggestions from code review
dciborow May 24, 2023
d207de8
Update setup.py
dciborow May 24, 2023
23cb1db
Update azdev/operations/testtool/tests/test_profile_context.py
dciborow May 24, 2023
ec84363
Apply suggestions from code review
dciborow May 24, 2023
513539f
Update ext_pylintrc
dciborow May 24, 2023
664c6c8
Update cli_pylintrc
dciborow May 24, 2023
074f8aa
Update .pylintrc
dciborow May 24, 2023
510f0c1
Apply suggestions from code review
dciborow May 24, 2023
f07822b
Update azdev/operations/help/__init__.py
bebound Jun 30, 2023
a4623ce
Update setup.py
bebound Jun 30, 2023
ae28381
Merge branch 'dev' into dciborow/pylint
bebound Jun 30, 2023
53f9b3d
Ignore missing-timeout
bebound Jun 30, 2023
4d2e2a5
Merge branch 'dev' into dciborow/pylint
bebound Jul 11, 2023
0e60fba
Minor fix
bebound Jul 12, 2023
f80f150
Enable deprecated-module
bebound Jul 12, 2023
08e6383
Remove useless config
bebound Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ disable=
too-few-public-methods,
too-many-arguments,
consider-using-f-string,
unspecified-encoding
unspecified-encoding,
# These rules were added in Pylint >= 2.12, disable them to avoid making retroactive change
broad-exception-raised,
deprecated-module,
Copy link
Contributor

@bebound bebound Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deprecated-module is quite useful and there is a few errors.
I prefer to enable it by default.

missing-timeout

[TYPECHECK]
# For Azure CLI extensions, we ignore some import errors as they'll be available in the environment of the CLI
Expand Down
21 changes: 14 additions & 7 deletions azdev/config/cli.flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,20 @@
max-line-length = 120
max-complexity = 10
ignore =
E501, # line too long, it is covered by pylint
E722, # bare except, bad practice, to be removed in the future
F401, # imported but unused, too many violations, to be removed in the future
F811, # redefinition of unused, to be removed in the future
C901 # code flow is too complex, too many violations, to be removed in the future
W503 # line break before binary operator
W504 # line break after binary operator effect on readability is subjective
# line too long, it is covered by pylint
E501,
Comment on lines +5 to +6
Copy link
Member

@jiasli jiasli Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for reformatting?

Copy link
Contributor

@bebound bebound Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because flake8 upgrade to 6.0. Flake8 6.0 does not support inline comments for any of the keys, so comments should be put in newline.
Use this to prevent ValueError: Error code '#' supplied to 'ignore' option does not match '^[A-Z]{1,3}[0-9]{0,3}$'

Related issue: PyCQA/flake8#1750 Azure/azure-cli#25370

Copy link
Contributor

@bebound bebound Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When pylint==2.11.1, it installs mccabe 0.6.1. flake8 also requires mccabe, and only 4.0.1 meet the version constraint.

  - pylint [required: ==2.11.1, installed: 2.11.1]
    - astroid [required: >=2.8.0,<2.9, installed: 2.8.6]
      - lazy-object-proxy [required: >=1.4.0, installed: 1.9.0]
      - setuptools [required: >=20.0, installed: 63.2.0]
      - wrapt [required: >=1.11,<1.14, installed: 1.13.3]
    - isort [required: >=4.2.5,<6, installed: 5.12.0]
    - mccabe [required: >=0.6,<0.7, installed: 0.6.1]
    - platformdirs [required: >=2.2.0, installed: 2.6.2]
    - toml [required: >=0.7.1, installed: 0.10.2]
 
- flake8 [required: Any, installed: 4.0.1]
    - mccabe [required: >=0.6.0,<0.7.0, installed: 0.6.1]
    - pycodestyle [required: >=2.8.0,<2.9.0, installed: 2.8.0]
    - pyflakes [required: >=2.4.0,<2.5.0, installed: 2.4.0]

After upgrading PyLint 2.7, mccabe becomes [required: >=0.6,<0.8, installed: 0.7.0], and flake8 becomes 6.0.0.

Copy link
Member

@jiasli jiasli Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this issue still exist in pylint 2.17.4? https://pypi.org/project/pylint/

Copy link
Contributor

@bebound bebound Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When run azdev style, it uses pylintrc and .flake8 in CLI/Extension repo folder. If the files do not exist, it use cli.xxxx or ext.xxx in azdev config folder.
CLI's .flake8 is updated in Azure/azure-cli#25370.

# bare except, bad practice, to be removed in the future
E722,
# imported but unused, too many violations, to be removed in the future
F401,
# redefinition of unused, to be removed in the future
F811,
# code flow is too complex, too many violations, to be removed in the future
C901,
# line break before binary operator
W503,
# line break after binary operator effect on readability is subjective
W504
exclude =
azure_cli_bdist_wheel.py
build
Expand Down
26 changes: 15 additions & 11 deletions azdev/config/cli_pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ disable=
useless-import-alias,
useless-suppression,
import-outside-toplevel,
wrong-import-order
wrong-import-order,
# These rules were added in Pylint >= 2.12, disable them to avoid making retroactive change
broad-exception-raised,
deprecated-module,
missing-timeout

[FORMAT]
max-line-length=120
Expand All @@ -50,13 +54,13 @@ min-similarity-lines=10
# The invalid-name checker must be **enabled** for these hints to be used.
include-naming-hint=yes

module-name-hint=lowercase (keep short; underscores are discouraged)
Copy link
Member

@jiasli jiasli Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually curious how (keep short; underscores are discouraged) works.

Copy link
Contributor

@bebound bebound Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hint is shown in the invalid-name error message. For example:
demo/demo.py:19:0: C0103: Class name "aa" doesn't conform to '{invalid-classname-hint}' pattern ('[_]{0,2}[A-Z]{1}[A-Za-z0-9]{0,30}$' pattern) (invalid-name)

I've removed all xxx-hint settings. See Azure/azure-cli#26685 (comment) and #359 (comment)

const-name-hint=UPPER_CASE_WITH_UNDERSCORES
class-name-hint=CapitalizedWords
class-attribute-name-hint=lower_case_with_underscores
attr-name-hint=lower_case_with_underscores
method-name-hint=lower_case_with_underscores
function-name-hint=lower_case_with_underscores
argument-name-hint=lower_case_with_underscores
variable-name-hint=lower_case_with_underscores
inlinevar-name-hint=lower_case_with_underscores (short is OK)
module-naming-style=snake_case
const-naming-style=UPPER_CASE
class-naming-style=PascalCase
class-attribute-naming-style=snake_case
attr-naming-style=snake_case
method-naming-style=snake_case
function-naming-style=snake_case
argument-naming-style=snake_case
variable-naming-style=snake_case
inlinevar-naming-style=snake_case
Copy link
Member

@jiasli jiasli Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the name change?

Copy link
Contributor

@bebound bebound Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a incompatibility introduced by pylint 2.14

Several occurrences of E0015: Unrecognized option found on the config file:
function-name-hint, variable-name-hint, const-name-hint, attr-name-hint, argument-name-hint, class-attribute-name-hint, inlinevar-name-hint, class-name-hint, module-name-hint, method-name-hint - This is where the incompatibility is: The old option names are rejected by pylint 2.14, and the new option names are rejected by older pylint versions.

Ref: pylint-dev/pylint#6931

Copy link
Member

@jiasli jiasli Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw the document has been moved to https://pylint.pycqa.org/en/v2.17.4/user_guide/messages/convention/invalid-name.html#predefined-naming-styles

Copy link
Contributor

@bebound bebound Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further investigation, these naming-style config can be removed.

  1. They are nearly the same as default value.
  2. invalid-name rule is disabled, these configurations make no sense.
  3. cli_pylintrc hardly take effect as CLI has its own pylintrc.

I've removed them in CLI.
Azure/azure-cli#26685 (comment)

21 changes: 14 additions & 7 deletions azdev/config/ext.flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,20 @@
max-line-length = 120
max-complexity = 10
ignore =
E501, # line too long, it is covered by pylint
E722, # bare except, bad practice, to be removed in the future
F401, # imported but unused, too many violations, to be removed in the future
F811, # redefinition of unused, to be removed in the future
C901 # code flow is too complex, too many violations, to be removed in the future
W503 # line break before binary operator
W504 # line break after binary operator effect on readability is subjective
# line too long, it is covered by pylint
E501,
# bare except, bad practice, to be removed in the future
E722,
# imported but unused, too many violations, to be removed in the future
F401,
# redefinition of unused, to be removed in the future
F811,
# code flow is too complex, too many violations, to be removed in the future
C901,
# line break before binary operator
W503,
# line break after binary operator effect on readability is subjective
W504
exclude =
*/vendored_sdks
docs
Expand Down
27 changes: 16 additions & 11 deletions azdev/config/ext_pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ disable=
too-many-arguments,
invalid-name,
duplicate-code,
import-outside-toplevel
import-outside-toplevel,
# These rules were added in Pylint >= 2.12, disable them to avoid making retroactive change
broad-exception-raised,
deprecated-module,
missing-timeout

[TYPECHECK]
# For Azure CLI extensions, we ignore some import errors as they'll be available in the environment of the CLI
Expand Down Expand Up @@ -45,13 +49,14 @@ min-similarity-lines=10
# The invalid-name checker must be **enabled** for these hints to be used.
include-naming-hint=yes

module-name-hint=lowercase (keep short; underscores are discouraged)
const-name-hint=UPPER_CASE_WITH_UNDERSCORES
class-name-hint=CapitalizedWords
class-attribute-name-hint=lower_case_with_underscores
attr-name-hint=lower_case_with_underscores
method-name-hint=lower_case_with_underscores
function-name-hint=lower_case_with_underscores
argument-name-hint=lower_case_with_underscores
variable-name-hint=lower_case_with_underscores
inlinevar-name-hint=lower_case_with_underscores (short is OK)
module-naming-style=snake_case
const-naming-style=UPPER_CASE
class-naming-style=PascalCase
class-attribute-naming-style=snake_case
attr-naming-style=snake_case
method-naming-style=snake_case
function-naming-style=snake_case
argument-naming-style=snake_case
variable-naming-style=snake_case
inlinevar-naming-style=snake_case

2 changes: 1 addition & 1 deletion azdev/operations/extensions/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def get_whl_from_url(url, filename, tmp_dir, whl_cache=None):
if url in whl_cache:
return whl_cache[url]
import requests
r = requests.get(url, stream=True)
r = requests.get(url, stream=True, timeout=10)
try:
assert r.status_code == 200, "Request to {} failed with {}".format(url, r.status_code)
except AssertionError:
Expand Down
14 changes: 7 additions & 7 deletions azdev/operations/help/refdoc/common/directives.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ def handle_signature(self, sig, signode):
signode += addnodes.desc_addname(sig, sig)
return sig

def needs_arglist(self): # pylint: disable=no-self-use
def needs_arglist(self):
return False

def add_target_and_index(self, name, sig, signode):
signode['ids'].append(name)

def get_index_text(self, modname, name): # pylint: disable=unused-argument, no-self-use
Copy link
Contributor

@bebound bebound Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise it raises azdev/operations/help/refdoc/common/directives.py:46:0: R0022: Useless option value for 'disable', 'no-self-use' was moved to an optional extension, see https://pylint.pycqa.org/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)

Copy link
Member

@jiasli jiasli Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pylint apparently doesn't think no-self-use is important anymore: pylint-dev/pylint#5502

def get_index_text(self, modname, name): # pylint: disable=unused-argument
return name


Expand All @@ -53,7 +53,7 @@ class CliGroupDirective(CliBaseDirective):
Field('docsource', label='Doc Source', has_arg=False,
names=('docsource', 'documentsource')),
Field('deprecated', label='Deprecated', has_arg=False,
names=('deprecated'))
names=('deprecated',))
])


Expand All @@ -63,23 +63,23 @@ class CliCommandDirective(CliBaseDirective):
Field('docsource', label='Doc Source', has_arg=False,
names=('docsource', 'documentsource')),
Field('deprecated', label='Deprecated', has_arg=False,
names=('deprecated'))
names=('deprecated',))
])


class CliArgumentDirective(CliBaseDirective):
doc_field_types = copy.copy(_CLI_FIELD_TYPES)
doc_field_types.extend([
Field('required', label='Required', has_arg=False,
names=('required')),
names=('required',)),
Copy link
Member

@jiasli jiasli Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the semantics of the code. Better to confirm if it is required or works as expected.

Copy link
Contributor

@bebound bebound Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

names should be Tuple[unicode, ...].
https://github.com/sphinx-doc/sphinx/blob/645044d7f03c7fcb4c39a8edf40aa66f6d6c2b64/sphinx/util/docfields.py#L55-L57

Why is this type error never found?

Copy link
Contributor

@bebound bebound Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I comfirm this is a typo, as next line it becomes a tuple:

azure-cli-dev-tools/azdev/operations/help/refdoc/common/directives.py

Lines 75 to 76 in cabbb3e

Field('values', label='Allowed values', has_arg=False,
names=('values', 'choices', 'options')),

Field('values', label='Allowed values', has_arg=False,
names=('values', 'choices', 'options')),
Field('default', label='Default value', has_arg=False,
names=('default')),
names=('default',)),
Field('source', label='Values from', has_arg=False,
names=('source', 'sources')),
Field('deprecated', label='Deprecated', has_arg=False,
names=('deprecated'))
names=('deprecated',))
])


Expand Down
2 changes: 1 addition & 1 deletion azdev/operations/performance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def _benchmark_cmd_staticstic(time_series: list):
avg_time = sum(time_series) / size

std_deviation = sqrt(
sum([(t - avg_time) * (t - avg_time) for t in time_series]) / size
sum((t - avg_time) * (t - avg_time) for t in time_series) / size
)

return {
Expand Down
2 changes: 1 addition & 1 deletion azdev/operations/tests/test_benchmark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def test_statistic_ok(self):
max_num = round(rands[-1], 4)
min_num = round(rands[0], 4)
avg_num = round(sum(rands) / len(rands), 4)
std_num = round(sqrt(sum([(t - avg_num) ** 2 for t in rands]) / len(rands)), 4)
std_num = round(sqrt(sum((t - avg_num) ** 2 for t in rands) / len(rands)), 4)

stats = _benchmark_cmd_staticstic(rands)

Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
'gitpython',
'jinja2',
'knack',
'pylint==2.11.1',
'pylint>=2.12,<3',
'pytest-xdist', # depends on pytest-forked
'pytest-forked',
'pytest>=5.0.0',
Expand Down