5
\$\begingroup\$

I am making some functions within a function for learning which when passed information about a named tuple: it returns a reference to a class object from which we can construct instances of the specified named tuple. Is there any way to make the code a bit cleaner? Any help would be great. The function pnamedtuple has other functions defined in it.

Here is my current functioning code:

import re, traceback, keyword
from goody import type_as_str

def pnamedtuple(type_name, field_names, mutable=False):
    '''Passed information about a named tuple: it returns a reference to a class 
    object from which we can construct instances of the specified named tuple.'''

    def show_listing(s):
        for i, l in enumerate(s.split('\n'), 1):
            print('{num: >3} {text}'.format(num=i, text=l.rstrip()))

    def unique_list(l):
        ulist = []
        for thing in l:
            if thing not in ulist:
                ulist.append(thing)
        return ulist


    regex = re.compile('^([a-zA-Z]{1}\w*)$')
    resplit = re.compile('[ ,]')
    if re.match(regex, str(type_name)) == None:
        raise SyntaxError('Illegal type name: ' + str(type_name))
    if type(field_names) not in (list, str):
        raise SyntaxError('Field names cannot be extracted: improper typing.' + str(field_names) + 'are not a list or a str, but instead: ' + type_as_str(field_names))
    if type(field_names) is str:
        fields = re.split(resplit, field_names)
        fields = [f for f in fields if f not in (None, '')]
    if type(field_names) is list:
        fields = field_names
    fields = unique_list(fields)
    for field in fields:
        if field in keyword.kwlist:
            raise SyntaxError('Field name: (' + str(field) + ') is a keyword and cannot be used.')
        if field[0].lower() not in 'abcdefghijklmnopqrstuvwxyz':
            raise SyntaxError('Field name: (' + str(field) + ') doesn\'t start with a letter')


    def init(field_names, mutable):
        '''has all the field names as parameters (in the order they appear in the 
        second argument to pnamedtuple) and initializes every instance name (using 
        these same names) with the value bound to its parameter. '''
        variables = ''
        for var in field_names:
            variables += 'self.' + str(var) + ' = ' + str(var) + '\n' + 8 * ' '
        str_init = \
        '''def __init__(self, {names}):
               {vars}
               self._fields = {fields}
               self._mutable = {mutate}
        '''
        return str_init.format(names=', '.join([field for field in field_names]), vars=variables, fields=str([field for field in field_names]), mutate=mutable)



    def prepr(type_name, field_names):
        ''' returns a string, which when passed to eval returns a newly constructed 
        object that has all the same instance names and values(==) as the object 
        __repr__ was called on.'''
        vars_repr = ''
        vars_self = ''
        for var in field_names:
            vars_repr += ',' + var + '={' + var + '}'
            vars_self += ',' + var + '=self.' + var 
        vars_repr = vars_repr.lstrip(',')
        vars_self = vars_self.lstrip(',')
        all_vars = vars_repr + ')".format(' + vars_self
        str_repr = \
        '''    def __repr__(self):
                   return "{name}({vars}) 
        '''   
        return str_repr.format(name=type_name, vars=all_vars)



    def get_each_var(field_names):
        get_var = ''
        for var in field_names:
            get_var += \
        '''    def get_{v}(self):
                   return self.{v}\n\n'''.format(v=var)
        return get_var



    def get_item(type_name, field_names):
        '''Overload the [] (indexing operator) for this class: an index of 0 returns 
        the value of the first field name in the field_names list; an index of 1 
        returns the value of the second field name. '''

        str_get = \
       '''    def __getitem__(self, index):
                 if type(index) not in (str, int):
                     raise IndexError('Index must be a str or an int: (' + str(index) + ')' + 'is a + str(type(index))' + '.')
                 if type(index) is str:
                    if index not in {fields}:
                        raise IndexError('Point.__getitem__: index('+index+') is illegal.')
                    else:
                         return self.__dict__[index]
                 else:
                      if index in range(len(self._fields)):
                           return self.__dict__[self._fields[index]]
                      else:
                           raise IndexError('Index (' + str(index) + ') is out of range.)')'''                    
        return str_get.format(fields=str([str(field) for field in field_names]))



    def equals():
        '''Overload the == operator so that it returns True when the two named tuples
        come from the same class and have all their name fields bound to equal 
        values. '''

        return\
        '''    def __eq__(self, right):
                   return repr(self) == repr(right)
        '''



    def replace():
        '''takes **kargs as a parameter (keyword args). This allows the name kargs to 
        be used in the method as a dict of parameter names and their matching 
        argument values. The semantics of the _replace method depends on the value 
        stored in the instance name self._mutable: If True, the instance namess of
        the object it is called on are changed and the method returns None. '''

        return\
        '''    def _replace(self,**kargs):
                   if self._mutable:
                      for key, value in kargs.items():
                          if key in self._fields:
                              self.__dict__[key] = value
                          else:
                               raise TypeError('Invalid key: (' + str(key) +') is not in a valid argument name' )
                      return               
                   else:
                       new_thing = eval(str(repr(self)))
                       for key, value in kargs.items():
                           if key in new_thing._fields:
                               new_thing.__dict__[key] = value
                           else:
                               raise TypeError('Invalid key: (' + str(key) +') is not in a valid argument name')
                       return new_thing                
 '''

    new_class = \
'''class {name}:
    {init}


{rep}


{get_each}


{get_the_thing}


{eq}


{repl}'''

    class_definition = new_class.format(name=type_name, init=init(fields, mutable), rep=prepr(type_name, fields), get_each=get_each_var(fields), get_the_thing=get_item(type_name, fields), eq=equals(), repl=replace())

# For initial debugging, always show the source code of the class
# show_listing(class_definition)

# Execute the class_definition string in a local name_space and bind the
#   name source_code in its dictionary to the class_defintion; return the
#   class object created; if there is a syntax error, list the class and
#   show the error
    name_space = dict(__name__='pnamedtuple_{type_name}'.format(type_name=type_name))
    try:
        exec(class_definition, name_space)
        name_space[type_name].source_code = class_definition
    except (TypeError, SyntaxError):
        show_listing(class_definition)
        traceback.print_exc()
    return name_space[type_name]



if __name__ == '__main__':
    import driver
    driver.driver()

This is an exercise to show how Python functions can define other Python code (in this case, a class) in an unexpected way: the function can build a huge string that represents the definition of a Python class and then call exec on it, which causes Python to define that class just as if it were written in a file and imported (in which case Python reads the file as a big string and does the same thing).

An example of this running code will be:

from pcollections import pnamedtuple as pnt
Point = pnt('Point', 'x y')
origin = Point(0,0)
p1 = Point(5,2)
print(p1)
   Point(x=5,y=2)
print(p1.get_x())
   5
print(p1[0])
   5
print(p1['x'])
   5
print(p1['z'])

Traceback (most recent call last):
  File "C:\Users\Pattis\workspace\courselib\driver.py", line 224, in driver
  exec(old,local,globl)
  File "", line 1, in 
  File "", line 17, in __getitem__
IndexError: Point.__getitem__: index(z) is illegal

p2 = p1._replace(x=2,y=5)
print(p1,p2)
   Point(x=5,y=2) Point(x=2,y=5)
\$\endgroup\$
  • \$\begingroup\$ Just so I am clear, - the point class is created outside pnamedtuple. That is, it already exists prior to the call. \$\endgroup\$ – ben rudgers Feb 6 '15 at 13:09
  • \$\begingroup\$ @benrudgers- that is correct. There is a class called Point outside pnamedtuple already created. I gave that as an example. \$\endgroup\$ – LucyBen Feb 6 '15 at 16:20
  • \$\begingroup\$ Ok, thanks. What does this code do that can't be done just by calling Point directly? A clearer picture will help find ways it might be better. \$\endgroup\$ – ben rudgers Feb 6 '15 at 20:48
  • \$\begingroup\$ @benrudgers- I've updated the question, provided an answer for your question above as well as give additional example. Hope this helps. \$\endgroup\$ – LucyBen Feb 7 '15 at 6:25
  • \$\begingroup\$ The code calls eval not exec as stated in the question's narrative. \$\endgroup\$ – ben rudgers Feb 9 '15 at 15:18
2
+50
\$\begingroup\$

Code Organization

The top of the source file is filled with items that don't help the reader understand the code: show_listing and unique_list don't flow from the high level descriptive comment. They're implementation details two or three levels of abstraction down.

What follows next is some purely procedural string bashing at the "top level". Refactoring this to its own function would make the code clearer and more modular. The names regex and resplit are not very descriptive.

Line 166 is 216 characters. I can't wrap my head around it. And it appears to be at the heart of the whole program. It seems like the sort of thing that might deserve it's own module.

Procedural Code

It's ok to organize procedural code procedurally. It helps if it the overall procedure is described at a very high level:

 # This is an explanatory form not necessarily the preferred implementation
 def pnamedtuple(type_name, field_names, mutable=False):
     input    = self.my_lex (type_name, field_names)
     parsed   = self.my_parse(input)
     compiled = self.my_compile(parsed)
     return   = self.my_eval(compiled)

Doing so provides a roadmap for reading the code. Readers can find what they are looking for and ignore what they aren't. Modularizing each step makes the interfaces explicit and abstracts implementation details into black boxes.

Eval

Code that calls eval has uses. It also opens up a meaningful risk vector for the injection of malicious code if there is any path by which user input is passed into it. If pnamedtuple is user facing e.g. if the user can enter values into fields, then it should be assumed that the user can run arbitrary code on the system hosting applications incorporating it.

There are cases where it may be possible to sanitize strings. But doing so requires nailing down the problem domain. See this StackOverflow QA. Python's ast module is safer.

Architecture

There are two big categories for this type of code.

  1. The classes that will be input are known ahead of time.
  2. The classes that will be input are not known ahead of time.

The first category tends to correlate with developing a domain specific language and boils down toward dispatch. For example a geometry engine for points (x, y, color) and lines (point_1, point_2) could insure that everything is red.

The second category tends toward something like a pre-processor and modifying and filtering input values. For example any time the string "17" is input it is replaced with the string "18" [not necessarily a good or sensible idea].

The code under review is structured like the second category. If it is intended for addressing problems where the types are known ahead of time, it could be structured differently and more safely using dictionaries of functions for dispatch.

\$\endgroup\$
2
\$\begingroup\$

Argh, code generation in Python. Well, in this case it seems safe as you're not dealing with user code except for the variable names.

So a few suggestions then:

  • I'd typically expect that field_names as a list instead of a string literal; it would matter if you were to use pnamedtuple from other functions, because then you'd have to construct the string from a list (probably) in many cases.
  • The dance with commas and string concatenation in prepr can be written more cleanly with join as you already did in init.
  • And if you're on that, why not use format all the time as well.
  • get_fieldname strikes me as very ugly. I'd really prefer just the keys themselves with the @property annotation.
  • I think that specifying multiple fields with the same name should be an error instead of you having to deal with unique_list.

Feature requests (I probably won't use it, but these seem like obvious additions to me):

  • Default values for fields. They would need to be literal values, or you'd have to specify them as strings I guess, but having e.g. 0 as default value for a vector tuple would be nice.
  • Deal with redefinition. This is a tricky one as you'd have to think about what happens with old instances (or you just throw an error instead).
  • Generate some documentation for the new class. At least the class itself could use a hint like "This class was automagically generated by pnamedtuple (at somefile.py:1234)."

All in all a nice application of metaprogramming, although I'm still not a huge fan (cue the meme) of a string-based approach.

\$\endgroup\$
  • 1
    \$\begingroup\$ @ferada- Thanks for all the suggestions. I'll definitely keep them in mind. Any chance of getting an improved version of the code? :) \$\endgroup\$ – LucyBen Feb 8 '15 at 20:06
  • \$\begingroup\$ Hi @LucyBen, if you liked this answer then maybe you can reward the poster with an upvote! \$\endgroup\$ – janos Feb 15 '15 at 11:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.