Skip to main content
Code Review

Return to Answer

added 2054 characters in body
Source Link
SylvainD
SylvainD
  • 29.8k
  • 1
  • 49
  • 93

Shallow review

self.call_re seems to be variable that does not change, is the same for all instance and is used only in __mul__.

You could define it as a "constant" or as a local variable in __mul__. The same thing applies to sep_re.

def __init__(self, *_):
    self.t = T()

def __mul__(self, code):
    CALL_RE = f'{self.__class__.__name__}\((.+)\).*'
    return CALL_RE * code.count(self.__class__.__name__)

def setattrs(self, t, varnames, f_back):
    SEP_RE = '\s*,\s*'
    for name in split(SEP_RE, varnames.strip(', ')):

I think that normalise_pos would be easier to understand if it was written like this:

def normalize_pos(self, no):
    self.__class__.no += 1
    if self.__class__.no == no:
        self.__class__.no = 0

in walk_f_back: if bool(unpacked) is True: can be re-written if unpacked:.

Also, Python does not handle deep recursions very well because it does not perform Tail Recursion Optimisation. If needed, you can rewrite the function:

def walk_f_back(f_back, packed):
    while True:
        unpacked = [unpacked_name for unpacked_name, v in f_back.f_locals.items() if v in f_back.f_locals[packed]]
        if unpacked:
            return unpacked
        f_back = f_back.f_back

The formatting in the tests make things hard to read for no good reason. It's ok to go a few characters above the 80-char limits if is makes the code better. There's nothing wrong with:

    with FrameHack(w, x) as t0:
        with FrameHack(y, z) as t1, FrameHack(*xy, w) as t2, FrameHack(w, z) as t3:

In setvarname, you can get rid of continue.

    if var is True:
        for varname in cached.copy():
            module.__dict__.pop(varname)
            cached.remove(varname)
    elif var[0] not in clean:
        setattr(module, *var)
        cached.add(var[0])

I'll try to have a real look at your code asap but I must confess it is quite hard to dive into that kind of things :)

I'll try to have a real look at your code asap but I must confess it is quite hard to dive into that kind of things :)

Shallow review

self.call_re seems to be variable that does not change, is the same for all instance and is used only in __mul__.

You could define it as a "constant" or as a local variable in __mul__. The same thing applies to sep_re.

def __init__(self, *_):
    self.t = T()

def __mul__(self, code):
    CALL_RE = f'{self.__class__.__name__}\((.+)\).*'
    return CALL_RE * code.count(self.__class__.__name__)

def setattrs(self, t, varnames, f_back):
    SEP_RE = '\s*,\s*'
    for name in split(SEP_RE, varnames.strip(', ')):

I think that normalise_pos would be easier to understand if it was written like this:

def normalize_pos(self, no):
    self.__class__.no += 1
    if self.__class__.no == no:
        self.__class__.no = 0

in walk_f_back: if bool(unpacked) is True: can be re-written if unpacked:.

Also, Python does not handle deep recursions very well because it does not perform Tail Recursion Optimisation. If needed, you can rewrite the function:

def walk_f_back(f_back, packed):
    while True:
        unpacked = [unpacked_name for unpacked_name, v in f_back.f_locals.items() if v in f_back.f_locals[packed]]
        if unpacked:
            return unpacked
        f_back = f_back.f_back

The formatting in the tests make things hard to read for no good reason. It's ok to go a few characters above the 80-char limits if is makes the code better. There's nothing wrong with:

    with FrameHack(w, x) as t0:
        with FrameHack(y, z) as t1, FrameHack(*xy, w) as t2, FrameHack(w, z) as t3:

In setvarname, you can get rid of continue.

    if var is True:
        for varname in cached.copy():
            module.__dict__.pop(varname)
            cached.remove(varname)
    elif var[0] not in clean:
        setattr(module, *var)
        cached.add(var[0])

I'll try to have a real look at your code asap but I must confess it is quite hard to dive into that kind of things :)

Source Link
SylvainD
SylvainD
  • 29.8k
  • 1
  • 49
  • 93

Disclaimer: I am not quite sure this can be considered as a proper review but this is definitly too big for a comment.

Not all variables are in f_locals

Not all variables that can be accessed from a frame are in frame.f_locals, you'll miss non-local variables which are:

  • enclosing variables
  • global variables
  • builtins

This is known as the LEGB Rule (Local Enclosing Global Builtin).

It is hard to tell whether this is relevant to the problem you are trying to solve but if you do, frame.f_globals and frame.f_builtins would interest you (I haven't found a way to access the variables in enclosing scope).

I've played with this as part of one of my pet project. Here's the parts that could be more relevant to you:

def merge_dict(*dicts):
    """Merge dicts and return a dictionnary mapping key to list of values.

    Order of the values corresponds to the order of the original dicts.
    """
    ret = dict()
    for dict_ in dicts:
        for key, val in dict_.items():
            ret.setdefault(key, []).append(val)
    return ret

ScopedObj = namedtuple('ScopedObj', 'obj scope')


def add_scope_to_dict(dict_, scope):
    """Convert name:obj dict to name:ScopedObj(obj,scope) dict."""
    return dict((k, ScopedObj(v, scope)) for k, v in dict_.items())


def get_objects_in_frame(frame):
    """Get objects defined in a given frame.

    This includes variable, types, builtins, etc.
    The function returns a dictionnary mapping names to a (non empty)
    list of ScopedObj objects in the order following the LEGB Rule.
    """
    # https://www.python.org/dev/peps/pep-0227/ PEP227 Statically Nested Scopes
    # "Under this proposal, it will not be possible to gain dictionary-style
    #      access to all visible scopes."
    # https://www.python.org/dev/peps/pep-3104/ PEP 3104 Access to Names in
    #      Outer Scopes
    # LEGB Rule : missing E (enclosing) at the moment.
    # I'm not sure if it can be fixed but if it can, suggestions
    # tagged TODO_ENCLOSING could be implemented (and tested).
    return merge_dict(
        add_scope_to_dict(frame.f_locals, 'local'),
        add_scope_to_dict(frame.f_globals, 'global'),
        add_scope_to_dict(frame.f_builtins, 'builtin'),
    )

I'll try to have a real look at your code asap but I must confess it is quite hard to dive into that kind of things :)