Skip to content

WIP: make pylint happy#561

Closed
pspacek wants to merge 1 commit into
rthalley:masterfrom
pspacek:pylint_module_enums
Closed

WIP: make pylint happy#561
pspacek wants to merge 1 commit into
rthalley:masterfrom
pspacek:pylint_module_enums

Conversation

@pspacek
Copy link
Copy Markdown
Collaborator

@pspacek pspacek commented Jul 31, 2020

pylint does not see Enum members exposed from modules via
globals().update(Algorithm.members)
This leads to tons of complains from pylint in third-party code which
uses trivial expressions like e.g. dns.flags.DO leads to error
E1101: Module 'dns.flags' has no 'DO' member (no-member)

This change exports Enum values statically so pylint can see them.
New test code verifies that all Enums in specified modules actually
export all its members so we omissions will be caught.

It re-introduces some duplication but I believe it is good tradeoff as
it allows static checkers to work again.

pylint does not see Enum members exposed from modules via
globals().update(Algorithm.__members__)
This leads to tons of complains from pylint in third-party code which
uses trivial expressions like e.g. dns.flags.DO leads to error
E1101: Module 'dns.flags' has no 'DO' member (no-member)

This change exports Enum values statically so pylint can see them.
New test code verifies that all Enums in specified modules actually
export all its members so we omissions will be caught.

It re-introduces some duplication but I believe it is good tradeoff as
it allows static checkers to work again.
@pspacek
Copy link
Copy Markdown
Collaborator Author

pspacek commented Jul 31, 2020

Hi @rthalley @bwelling! I realize this is a bit controversial but I cannot see other way to enable static code checkers to work with dnspython 2.y.z.

What do you think?
Should I invent more time into this and re-export all Enums which currently depend on globals().update(Algorithm.__members__) trick?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 31, 2020

Codecov Report

Merging #561 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #561   +/-   ##
=======================================
  Coverage   98.31%   98.32%           
=======================================
  Files         108      108           
  Lines        8032     8062   +30     
=======================================
+ Hits         7897     7927   +30     
  Misses        135      135           
Impacted Files Coverage Δ
dns/flags.py 100.00% <100.00%> (ø)
dns/opcode.py 96.77% <100.00%> (+0.47%) ⬆️
dns/rcode.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f03c793...44aff43. Read the comment docs.

@pspacek
Copy link
Copy Markdown
Collaborator Author

pspacek commented Jul 31, 2020

Example of pylint failures caused by "too much dynamic magic" in 2.0.0: https://gitlab.nic.cz/knot/deckard/pipelines/66674/failures

@rthalley
Copy link
Copy Markdown
Owner

First, let me say I hear what you're saying, and it's a good point. Introducing noise into other people's static checking is not something I want to do. I have traditionally used very high warning levels and warnings-are-errors in other projects, and it has been very helpful. And if you're doing that, you really want "zero warnings" to be the norm, as otherwise you miss the content in the noise. On the other hand, I have been deeply annoyed with pylint many times. Probably I should do another pass to get dnspython back to zero warnings.

I think there are three basic approaches:

  1. duplicate definitions by hand, and put something into the continuous integration to programmatically check there is no difference between the enums and the duplicated constants.

  2. duplicate definitions programmatically with code generation, either that edits files with enums or which generates a bunch of imports in a separate module, e.g. _rdatatype_constants.py. This too would need checking in the continuous integration as otherwise we could forget to run it and have skew.

  3. encourage people who care about warnings to update their constants with the enums

I don't like 3), but am listing it for completeness. I don't like it as it's making N people cope with noise instead of just us, so it wastes more time. 1) and 2) are pretty much equally ugly and have the same danger level. 2) might be less tedious, though it might still take longer :)

@rthalley
Copy link
Copy Markdown
Owner

Example generator code

from importlib import import_module

enum_names = [
    'dns.dnssec.Algorithm',
    'dns.dnssec.DSDigest',
    'dns.dnssec.NSEC3Hash',
    'dns.edns.OptionType',
    'dns.message.MessageSection',
    'dns.opcode.Opcode',
    'dns.rcode.Rcode',
    'dns.rdataclass.RdataClass',
    'dns.rdatatype.RdataType',
    'dns.update.UpdateSection',
]

for enum_name in enum_names:
    dot = enum_name.rindex('.')
    module_name = enum_name[:dot]
    type_name = enum_name[dot + 1:]
    mod = import_module(module_name)
    enum = getattr(mod, type_name)
    mname = module_name.lower()[4:]  # strip off 'dns.' too
    tname = type_name.lower()
    with open(f'dns/constants/_{mname}_{tname}.py', 'w') as f:
        print('# Copyright (C) Dnspython Contributors, see LICENSE ' +
              'for text of ISC license', file=f)
        print('#\n# This is a generated file, do not edit.\n', file=f)
        for t in enum:
            print(f'{t.name} = {t.value}', file=f)

@rthalley
Copy link
Copy Markdown
Owner

(The generator code is stuffing all the generated files into a "constants" directory so I don't have the listing of the "dns" directory cluttered by them.)

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.

3 participants