Skip to content

Constants generation at end of file approach#564

Merged
rthalley merged 6 commits into
masterfrom
constants2
Aug 4, 2020
Merged

Constants generation at end of file approach#564
rthalley merged 6 commits into
masterfrom
constants2

Conversation

@rthalley
Copy link
Copy Markdown
Owner

@rthalley rthalley commented Aug 3, 2020

Here's code generation and auditing that just adds aliases for the enums to the end of the file.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 3, 2020

Codecov Report

Merging #564 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
+ Coverage   98.33%   98.39%   +0.06%     
==========================================
  Files         108      108              
  Lines        8027     8170     +143     
==========================================
+ Hits         7893     8039     +146     
+ Misses        134      131       -3     
Impacted Files Coverage Δ
dns/dnssec.py 96.95% <100.00%> (+0.15%) ⬆️
dns/edns.py 100.00% <100.00%> (ø)
dns/flags.py 100.00% <100.00%> (ø)
dns/message.py 98.99% <100.00%> (+<0.01%) ⬆️
dns/opcode.py 96.77% <100.00%> (+0.47%) ⬆️
dns/rcode.py 100.00% <100.00%> (ø)
dns/rdataclass.py 100.00% <100.00%> (ø)
dns/rdatatype.py 100.00% <100.00%> (ø)
dns/rdtypes/dnskeybase.py 100.00% <100.00%> (ø)
dns/update.py 100.00% <100.00%> (ø)
... and 2 more

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 049eedd...c33265d. Read the comment docs.

@pspacek
Copy link
Copy Markdown
Collaborator

pspacek commented Aug 4, 2020

As far as I can tell this makes pylint and mypy happy again.

Nevertheless I suggest to pull in also tests from https://github.com/rthalley/dnspython/pull/561/files#diff-502f75666576bfedda27c30bfead1f9d . IMHO it is not a good idea to use the same code to generate the constants and also to check it. Test from PR #561 should independently verify that something terrible did not happen when generating the constants.

Comment thread util/constants-tool Outdated
module_name = enum_name[:dot]
type_name = enum_name[dot + 1:]
mod = import_module(module_name)
enum = getattr(mod, type_name)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This name parsing is duplicate of lines 25-32. Worth splitting it into a function?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I'm just getting rid of the whole check() function as the independent test you suggest is a good idea. I took your code and made just a slight change to it so that we can ignore the things in dns.dnssec that are ONLY enums (i.e. NSEC3Hash and DSDigest) and don't have module-level aliases.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I've also put the tests into their own module, test_constants.py

@rthalley rthalley merged commit 11432e4 into master Aug 4, 2020
@rthalley rthalley deleted the constants2 branch August 4, 2020 13:11
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