Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#1866 closed defect (fixed)

isc.dns constants like RRType.A() should be constants, not functions

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130219
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 7.74 Internal?: no

Description

In the C++ counterpart we intentionally define them via proxy
functions like RRType::A() to avoid static initialization fiasco.
But there's no such reason for the Python version, and using a
function looks awkward (and would be less efficient). These should be
defined as normal constant in the C++ wrappers.

Those include: RRType, RRClass, Opcode, and Rcode (and perhaps
more).

Subtickets

Attachments (1)

conv.py (1.0 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by jelte

  • Milestone Next-Sprint-Proposed deleted

comment:2 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:5 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed
  • Priority changed from medium to high

comment:7 Changed 7 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:8 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130205

comment:9 Changed 7 years ago by jinmei

  • Priority changed from high to medium

comment:10 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from new to accepted

comment:11 Changed 7 years ago by jinmei

trac1866 is ready for review.

As can be expected, the size of the diff is quite large, but the
changes should be straightforward.

The branch can be divided into several stages:

  • from 62bb1c4 to 3f74193 are the main part of the branch: I've extended the gen-rdatacode script so we can more easily make the C++ and Python definitions consistent. (but for Opcde and Rcode I rather chose hardcoding at the risk of repeating ourselves as it should be very unlikely to see more cases)
  • from 56e9d0e to d985d0d are the trivial adjustments to other python code that used the old definition. I've done this using the attached script; although many files were updated I believe it's easy to be confident about the changes if you see the script. taking and reviewing the actual diff would be still useful, though, and I also believe it looks quite trivial.
  • the rest (0479cb5 and beyond) is an optional extension: I've also solved the issue of #2409 as I found it possible with a small additional extension on top of this branch and the branch so far should be easy to understand.

But if the last part is controversial or the branch looks too big even
without it, I'm okay with differing that part.

Proposed changelog/news entry:

562.?	[func]*		jinmei
	libdns++/Python isc.dns: In Python isc.dns, function style
	constants for RRType, RRClass, Rcode and Opcode were deprecated
	and replaced with straightforward object constants, e.g., from
	RRType.AAAA() to RRType.AAAA.  This is a backward incompatible
	change.  Also, these constants are now more consistent between C++
	and Python, and RRType constants for all currently standardized
	types are now supported (even if Rdata for these are not yet
	available).
	(Trac #1866 and #2409, git TBD)

(This includes the additional extension part)

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

Changed 7 years ago by jinmei

comment:13 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:14 follow-ups: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

First, would it make sense to put the script to convert into the repository, let's say to the tools directory? In case some user already used our libraries, it might be useful for them. We should, of course, make it clear it is not supported in any way.

I think the convention how to keep empty git directories is to place a .keep file there, not gitignore. The .gitignore has special meaning for git and it feels a bit like an abuse.

The tests fail for me:

Running test: session_tests.py
...F..........F..........................
======================================================================
FAIL: test_convert_rrset_class (__main__.SessionModuleTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/bind10-1/bind10-20121219/_build/../src/lib/python/isc/ddns/tests/session_tests.py", line 136, in test_convert_rrset_class
    str(rrset3))
AssertionError: 'www.example.org. 3600 NONE A \\# 4 c0000201\nwww.example.org. 3600 NONE A \\# 4 [truncated]... != 'www.example.org. 360
0 CLASS254 A \\# 4 c0000201\nwww.example.org. 3600 CLASS254 [truncated]...
- www.example.org. 3600 NONE A \# 4 c0000201
?                       ^^^^^
+ www.example.org. 3600 CLASS254 A \# 4 c0000201
?                       ^^ +++++++
- www.example.org. 3600 NONE A \# 4 c0000202
?                       ^^^^^
+ www.example.org. 3600 CLASS254 A \# 4 c0000202
?                       ^^ +++++++


======================================================================
FAIL: test_convert_rrset_class (__main__.SessionTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/bind10-1/bind10-20121219/_build/../src/lib/python/isc/ddns/tests/session_tests.py", line 405, in test_convert_rrset_class
    str(rrset3))
AssertionError: 'www.example.org. 3600 NONE A \\# 4 c0000201\nwww.example.org. 3600 NONE A \\# 4 [truncated]... != 'www.example.org. 3600 CLASS254 A \\# 4 c0000201\nwww.example.org. 3600 CLASS254 [truncated]...
- www.example.org. 3600 NONE A \# 4 c0000201
?                       ^^^^^
+ www.example.org. 3600 CLASS254 A \# 4 c0000201
?                       ^^ +++++++
- www.example.org. 3600 NONE A \# 4 c0000202
?                       ^^^^^
+ www.example.org. 3600 CLASS254 A \# 4 c0000202
?                       ^^ +++++++

It is not code from the branch, but this regexp seems to be wrong (missing a dot after the backslash). It is in the generator script:

elif re.search('\cc$', file):

Why do you generate the code into a variable and accumulate it across all the RRtypes and then dump it after each new one? This seems like a quadratic run time and it looks unnecessary. Would it make sense to output it just once at the end?

Would it be more lightweight and easier to read if the list of „fake“ types (the ones that don't exist or we don't have them implemented yet) would be by a single file with a list instead of many effectively empty files?

comment:15 in reply to: ↑ 14 Changed 7 years ago by jinmei

Replying to vorner:

Some points first:

The tests fail for me:

Running test: session_tests.py
...F..........F..........................
======================================================================
FAIL: test_convert_rrset_class (__main__.SessionModuleTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/bind10-1/bind10-20121219/_build/../src/lib/python/isc/ddns/tests/session_tests.py", line 136, in test_convert_rrset_class

This doesn't happen to me. Are you sure to run it with the latest
isc.dns wrapper? Does it pass if you start from make clean?

Why do you generate the code into a variable and accumulate it across all the RRtypes and then dump it after each new one? This seems like a quadratic run time and it looks unnecessary. Would it make sense to output it just once at the end?

I don't understand this comment. Specifically which part of the code
are you referring to?

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by jinmei

Thanks for the review.

Replying to vorner:

First, would it make sense to put the script to convert into the repository, let's say to the tools directory? In case some user already used our libraries, it might be useful for them. We should, of course, make it clear it is not supported in any way.

I don't necessarily oppose to that, but at this time I'd rather avoid
including mostly-unused file. Realistically, I suspect there are yet
very few people using this Python library for development other than
ourselves.

I think the convention how to keep empty git directories is to place a .keep file there, not gitignore. The .gitignore has special meaning for git and it feels a bit like an abuse.

Okay, but I've actually removed the directory with the dot- file
based on your other comment below, so this point is now moot.

The tests fail for me:

Running test: session_tests.py
...F..........F..........................
======================================================================
FAIL: test_convert_rrset_class (__main__.SessionModuleTests)

As I responded separately, I couldn't reproduce it, and I suspect it
was because your build environment is incomplete.

It is not code from the branch, but this regexp seems to be wrong (missing a dot after the backslash). It is in the generator script:

elif re.search('\cc$', file):

Ah, right, fixed.

Why do you generate the code into a variable and accumulate it across all the RRtypes and then dump it after each new one? This seems like a quadratic run time and it looks unnecessary. Would it make sense to output it just once at the end?

I guess you (at least) meant generate_typeclasscode(), where we built
the auto-generated code by extending a string object. I'm not sure if
the internal implementation is that inefficient, and even if so, if it
matters in this usage. But I revised the code to what I guess you
would have envisioned.

Would it be more lightweight and easier to read if the list of „fake“ types (the ones that don't exist or we don't have them implemented yet) would be by a single file with a list instead of many effectively empty files?

Hmm, I once considered that approach and at that time it seemed to
require heavier updates to the generator script. But on looking at it
again it seems to be quite straightforward. So I adopted the idea
(but instead of introducing a separate file, simply listing them in
the generator script), removing the placeholder files.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I don't necessarily oppose to that, but at this time I'd rather avoid
including mostly-unused file. Realistically, I suspect there are yet
very few people using this Python library for development other than
ourselves.

Hmm, OK. It does make some sense not to put it to the git repo, as we'd have it there for long time (since we would forget). But maybe noting in the changelog a script can be downloaded from the trac site, at least? I know there are not many people who use it, but there might be few.

The tests fail for me:

Running test: session_tests.py
...F..........F..........................
======================================================================
FAIL: test_convert_rrset_class (__main__.SessionModuleTests)

As I responded separately, I couldn't reproduce it, and I suspect it
was because your build environment is incomplete.

I don't know, but it does not fail now. So I guess it must have been something like this, though I usually start with completely clean checkout for each review now (I have a script that checks out a new directory and does full build and tests).

Why do you generate the code into a variable and accumulate it across all the RRtypes and then dump it after each new one? This seems like a quadratic run time and it looks unnecessary. Would it make sense to output it just once at the end?

I guess you (at least) meant generate_typeclasscode(), where we built
the auto-generated code by extending a string object. I'm not sure if
the internal implementation is that inefficient, and even if so, if it
matters in this usage. But I revised the code to what I guess you
would have envisioned.

I did mean that one, yes. However, it might have been that I misunderstood the code, I though the generate_typeclasscode was called for each type and each class (so many many times), each time overwriting the file. I wasn't really worried about the speed of the in-memory string handling, just about the writes to the file.

But reading it again, it seems it is called just once for all types and once for all classes, so it doesn't matter. But maybe this way is simpler to read anyway.

Would it be more lightweight and easier to read if the list of „fake“ types (the ones that don't exist or we don't have them implemented yet) would be by a single file with a list instead of many effectively empty files?

Hmm, I once considered that approach and at that time it seemed to
require heavier updates to the generator script. But on looking at it
again it seems to be quite straightforward. So I adopted the idea
(but instead of introducing a separate file, simply listing them in
the generator script), removing the placeholder files.

Thanks

It looks OK to merge now, I think.

comment:19 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 4.08

comment:20 in reply to: ↑ 18 Changed 7 years ago by jinmei

Replying to vorner:

It looks OK to merge now, I think.

Okay, thanks, merge done. I've mentioned the conversion script in
ChangeLog.

Now closing ticket.

comment:21 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 4.08 to 3.74

comment:22 Changed 7 years ago by jinmei

  • Total Hours changed from 3.74 to 7.74
Note: See TracTickets for help on using tickets.