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

Pylint 2.12 #245

Merged
merged 9 commits into from
Mar 24, 2022
Merged

Pylint 2.12 #245

merged 9 commits into from
Mar 24, 2022

Conversation

stanislavlevin
Copy link
Contributor

@stanislavlevin stanislavlevin commented Feb 15, 2022

disabled:

  • consider-using-f-string
  • redundant-u-string-prefix
  • use-dict-literal
  • unspecified-encoding
  • use-list-literal

enabled:

  • useless-suppression

Fixes: #244

@stanislavlevin stanislavlevin force-pushed the pylint_2.12 branch 2 times, most recently from 5bbaa1f to 1adcc4d Compare February 15, 2022 12:45
@rcritten
Copy link
Collaborator

Thanks for this considerable patch. The changes look valid to me, I just find some of the new linting to be annoying :-)

Particularly the open() with encoding. Is that really necessary? I'm not really the best to judge this as I'm English-only whose alphabet consists only in ASCII so I may have lucked out in the encoding assumptions. Is my assumption correct that we're only dealing with plain ASCII or is it just safer/defensive programming to be explicit about it? Can we safely ignore this instead of extending the open arguments?

@stanislavlevin
Copy link
Contributor Author

You might want to read https://www.python.org/dev/peps/pep-0597.

In a few words:
The assumption about utf8 as the default encoding while opening files in text mode (default mode) may be wrong under certain conditions.

open(filename) isn't explicit about which encoding is expected:

If ASCII is assumed, this isn't a bug, but may result in decreased performance on Windows, particularly with non-Latin-1 locale encodings
If UTF-8 is assumed, this may be a bug or a platform-specific script
If the locale encoding is assumed, the behavior is as expected (but could change if future versions of Python modify the default)
From this point of view, open(filename) is not readable code.

Some of the opening files are assumed ASCII-only, some - not.
In my opinion, it's safer to make encoding explicit. For example, some of the changed by this PR files like configs may contain comments in non-ASCII made by admins (in russian for example). In this case successful opening of such files depends on locale.getpreferredencoding, which may or may not be 'UTF8' (depends on platform/cli options/env vars/etc.).

If you would like to say "I support only UTF8 platforms", then this Pylint check can be ignored.

@stanislavlevin
Copy link
Contributor Author

Another point is waiting for updated pylintrc in freeipa (I will open PR in a day or two) since pylintrc was force replaced with freeipa's one and thereby healthcheck inherits many skipped checks :(

@rcritten
Copy link
Collaborator

I had read the PEP which led me to the question. It didn't really clarify things as you have. I'm ok with this. I'll hold off for your further pylint changes.

And yeah, honestly, you know pylint way better than I do hence the copying from a working, similar project.

@stanislavlevin stanislavlevin force-pushed the pylint_2.12 branch 2 times, most recently from 94a8c92 to c795594 Compare February 25, 2022 09:28
@stanislavlevin
Copy link
Contributor Author

I synced to freeipa configuration, most of the new checkers have been skipped.

@rcritten
Copy link
Collaborator

Just to make sure I understand, this is synced to freeipa PR freeipa/freeipa#6196 ?

@stanislavlevin
Copy link
Contributor Author

let me recheck please.

Pylint 2.10 introduced new checker `redundant-u-string-prefix`:
> Used when we detect a string with a u prefix. These prefixes were
  necessary in Python 2 to indicate a string was Unicode, but since Python
  3.0 strings are Unicode by default.

There are ~40 emitted warnings right now. They can be fixed on
refactorings without any rush.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
- f-strings are not mandatory
- format can be more readable
- there are 140 spotted issues

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
Pylint 2.10 introduced new checkers:
> Emitted when using dict() to create an empty dictionary instead of the
  literal {}. The literal is faster as it avoids an additional function
  call.

> Emitted when using list() to create an empty list instead of the
  literal []. The literal is faster as it avoids an additional function
  call.

Too many unessential changes.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
Pylint 2.10 introduced new checker:
> It is better to specify an encoding when opening documents. Using the
  system default implicitly can create problems on other operating
  systems. See https://www.python.org/dev/peps/pep-0597/

According to that PEP:
> open(filename) isn't explicit about which encoding is expected:
  - If ASCII is assumed, this isn't a bug, but may result in decreased
    performance on Windows, particularly with non-Latin-1 locale
    encodings
  - If UTF-8 is assumed, this may be a bug or a platform-specific script
  - If the locale encoding is assumed, the behavior is as expected (but
    could change if future versions of Python modify the default)

IPA requires UTF-8 environments.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
https://pylint.pycqa.org/en/latest/user_guide/message-control.html#detecting-useless-disables:

> As pylint gets better and false positives are removed, disables that
  became useless can accumulate and clutter the code. In order to clean
  them you can enable the useless-suppression warning.

This doesn't enforce useless-suppression warnings as errors. The idea is
cleanup of these warings on every Pylint's bump.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
Either Fedora's or PyPI's release of Pylint can break
ipa-healthcheck's CI. With this change only maintainers of
ipa-healthcheck can decide which version of Pylint the project
is compatible with.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
Pylint 2.12.0 introduced new checker:
> Used when Pylint detects that collection literal comparison is being
  used to check for emptiness; Use implicit booleaness insteadof a
  collection classes; empty collections are considered as false

Comparison of variable to equality to collection:
> Lexicographical comparison between built-in collections works as follows:
  For two collections to compare equal, they must be of the same type,
  have the same length, and each pair of corresponding elements must
  compare equal (for example, [1,2] == (1,2) is false because the type is
  not the same).
  Collections that support order comparison are ordered the same as their
  first unequal elements (for example, [1,2,x] <= [1,2,y] has the same
  value as x <= y). If a corresponding element does not exist, the shorter
  collection is ordered first (for example, [1,2] < [1,2,3] is true).

So, `assert value == {}` is not the same as `assert not value`.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
Pylint 2.9 introduced new check:
> New checker consider-using-dict-items. Emitted when iterating over
dictionary keys and then indexing the same dictionary with the key
within loop body.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
This is backport of
freeipa/freeipa@a1f0f27.

Fixes: freeipa#244
Signed-off-by: Stanislav Levin <[email protected]>
@stanislavlevin
Copy link
Contributor Author

@rcritten, added 3 more commits to sync pylint_plugins.py and pylintrc. Now they are exactly the same as IPA's ones in today's dev branch.

@rcritten
Copy link
Collaborator

Sorry for the delay, been buried with work. I'll try to get to this soon. A quick look turned up no issues.

@rcritten
Copy link
Collaborator

LGTM, thanks for the contribution!

@rcritten rcritten added the ack label Mar 24, 2022
@rcritten rcritten merged commit 075be73 into freeipa:master Mar 24, 2022
@stanislavlevin
Copy link
Contributor Author

@rcritten, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pylint 2.12 issues
2 participants