Skip to content
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.

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

Bump Pylint to 2.17 and flake8 to 6.0 #359

Merged
merged 30 commits into from
Jul 14, 2023
Merged

Conversation

dciborow
Copy link
Contributor

@dciborow dciborow commented Mar 10, 2023

Fixes: #377 Update dependencies to become compatible with Python 3.11

This updates the previous start to this fix here - #351

Pylint and flake8 are bumped together, see #359 (comment)

@dciborow dciborow mentioned this pull request Mar 10, 2023
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
@dciborow dciborow changed the title Dciborow/pylint Bump Pylint to latest version May 24, 2023
azdev/config/ext_pylintrc Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@bebound bebound changed the title Bump Pylint to latest version Bump Pylint to 2.17 Jul 11, 2023
Comment on lines +5 to +6
# line too long, it is covered by pylint
E501,
Copy link
Member

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

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

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

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

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.

.pylintrc Outdated
@@ -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 and are disabled to avoid making retroactively required
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is not grammatically correct and hard to understand.

Comment on lines 57 to 66
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

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

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

Choose a reason for hiding this comment

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

Copy link
Contributor

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)

])


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

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

Choose a reason for hiding this comment

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

Copy link
Contributor

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:

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

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

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

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

.pylintrc Outdated
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

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.

@@ -50,13 +53,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

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

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)

@bebound bebound changed the title Bump Pylint to 2.17 Bump Pylint to 2.17 and flake8 to 6.0 Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated pylint breaks install
4 participants