Re: Trouble with new tracker designator?

Hi Tom:

In message <MWHPR08MB29417E5161A8C1E4BE1587FBCA7B0@MWHPR08MB2941.namprd08.prod.
outlook.com>,
Tom Ekberg writes:
>Thank you for your well thought-out response. Also I didn't check,
>I'm pretty sure that 'zip2py' would fail looking for object ID 2 of class zip.

I can't see how it would work. If it didn't zippy2345 would certainly
fail to be correctly parsed.

>According to a google search, a Python identifier (e.g. Python class
>name) starts with A-Za-z or _, and is followed by 0 or more
>A-Za-z0-9_. For roundup class names that changes since it doesn't end
>with a digit. Likewise a designator is a class name followed by one
>or more digits (see the roundup-admin help for retire). Here is a
>short unittest program that tests various cases using re:
>
>[...]
>class_re = r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)$'
>designator_re = r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)(\d+)$'
>
> TEST_CASES =3D [('zip2py44', 'designator', ('zip2py', '44')),
>         ('zip2py', 'class', ('zip2py',)),
>         ('zippy2', 'designator', ('zippy', '2')),
>         ('a9', 'designator', ('a', '9')),
>         ('a1234', 'designator', ('a', '1234')),
>         ('a', 'class', ('a',)),
>         ('_', 'class', ('_',)),
>         ] [...]
>
>If splitDesignator in hyperdb.py were changed to use designator_re
>(above) it would do a better job of detecting valid and invalid
>designators. Note that that function uses re.match, which only has an
>anchor on the first character of the string.

I am not quite sure what you mean by only an anchor on the first
character. designator_re has both ends anchored. I think to support
"zip2py" as a class name you have to have both anchors or the re will
end at the first 2.

I did a little performance testing with the various possible re's
and a subset of the cases you tested.

import re
import timeit

strings = ["issue234", "issue", "issue2", "issue2345678",
                   "2issue234", "is2ue234"]

for exp in [ r'([^\d]+)(\d+)', r'^(\w+?)(\d+)$',
             r'^(\w+?)(\d+)', r'(\w+?)(\d+)$',
             r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)(\d+)$' ]:


  print( "re: %s\n"%exp)
  print(timeit.timeit('re.compile(exp)', setup='import re; exp=r"%s"'%exp))
  dre=re.compile(exp)
  for designator in strings:

    print(timeit.timeit('dre.match(d)', setup='import re; \
                   dre=re.compile(r"%s"); d="%s"'%(exp, designator)))
    m = dre.match(designator)
    if m is None:
       print('"%s" not a node designator' % designator)
    else:
        print('"%s", class: %s, item: %s'%(designator,
                                           m.group(1), m.group(2)))
  print( "-------\n")

performance for the expected case pattern of issue234 I see:

====
re: ([^\d]+)(\d+) -- current re
1.74087810516 -- re.compile time in seconds 1,000,000 runs
1.00818586349 -- run time for matching pattern 1,000,000 runs
"issue234", class: issue, item: 234

re: ^(\w+?)(\d+)$ -- better re that allows zip2py as classname
1.75750112534
1.3666009903
"issue234", class: issue, item: 234

re: ^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)(\d+)$ -- best your re 
1.80379509926
1.46494102478
"issue234", class: issue, item: 234
=====

Compiling the better re is .9% slower and your best re is 3.6% worse
than current.

Matching is 35% worse for the better re and 45% worse for your best
re. I don't have good stats on how often splitDesignator is called and
how much of an impact a 45% slowdown would cause. Trying to use
cProfile and pstats isn't exactly clear.

>Likewise, change the
>constructor (__init__) method for hyperdb.Class (roundup/hyperdb.py
>line 901). Something like:
>
> if not re.match(r'^([A-Za-z_](?:[A-Za-z_0-9]*[A-Za-z_]+)?)$', classname):
>    raise ValueError('Class name %s is not valid. It must start with a letter and not end with a digit.' % (classname,))

Technically _ is not a letter. I wonder if we should keep classes
starting with _ in reserve for future use (hidden/internal class for
whatever that means).

However if we properly limit class names in Class::__init__ I wonder
if we can simplify the re in splitDesignator assuming that the
designator has a classname of initial alpha, alphanumerics (plus _)
and no trailing digits. That still leaves us with a 35% slowdown over
the current re which doesn't support embedded numbers in classnames
8-(.

>Maybe define a function which validates a class name, and another
>function to validate a designator, to make it easier to write a test case.

We already have splitDesignator to validate designators. I thought
adding validateClass to the class "Class" would allows us to sprinkle
it into other class (like IssueClass, FileClass). It looks like those
classes are derived in backends from Class, so I think this would
work.  However it looks like all of these end up calling
Class::__init__ in hyperdb via:

   Class.__init__()

So do we need a validateClass or can I just add the re check to
hyperdb.py::Class::__init__ to line 901 like you suggested earlier?

Also Class::__init__ is only called once for each class while
splitDesignator may be called multiple times per run.

>I changed my class name to zippytwo to avoid the whole issue.

What no zippy_II? Roman numerals are "in"... 8-).x

Have a great week.

--
				-- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.