Hacker News new | ask | show | jobs
by abuckenheimer 2904 days ago
I don't know the Kirill Balunov example is pretty beautiful/simple/flat/readable

    if reductor := dispatch_table.get(cls):
        rv = reductor(x)
    elif reductor := getattr(x, "__reduce_ex__", None):
        rv = reductor(4)
    elif reductor := getattr(x, "__reduce__", None):
        rv = reductor()
    else:
        raise Error("un(shallow)copyable object of type %s" % cls)
especially when you compare it to the existing implementation:

    reductor = dispatch_table.get(cls)
    if reductor:
        rv = reductor(x)
    else:
        reductor = getattr(x, "__reduce_ex__", None)
        if reductor:
            rv = reductor(4)
        else:
            reductor = getattr(x, "__reduce__", None)
            if reductor:
                rv = reductor()
            else:
                raise Error("un(shallow)copyable object of type %s" % cls)
3 comments

Nah, write a little gadget:

    def f(F, *args):
      f.reductor = F(*args)
      return bool(f.reductor)

    if f(dispatch_table.get, cls)):
        rv = f.reductor(x)
    elif f(getattr, x, "__reduce_ex__", None):
        rv = f.reductor(4)
    elif f(getattr, x, "__reduce__", None):
        rv = f.reductor()
    else:
        raise Error("un(shallow)copyable object of type %s" % cls)

Same pattern works for re.match objects.

Once you abstract the patterns you can drive towards data-driven code:

    K = (
      (dispatch_table.get, (cls,), (x,)),
      (getattr, (x, "__reduce_ex__", None), (4,)),
      (getattr, (x, "__reduce__", None), ()),
    )

    for F, a, b in K:
      reductor = F(*a)
      if reductor:
        rv = reductor(*b)
        break
    else:
        raise Error("un(shallow)copyable object of type %s" % cls)

Syntax is the enemy, never substitute syntax for thought.
Your second example is great except that any error messages that get raised from inside getattr are going to be inscrutable. The traceback will be uninformative, and you'll be forced to drop into a debugger to figure out what's going on.

At runtime, your second example is actually significantly less explicit because so much is hidden away in mutable state.

> Your second example is great except that any error messages that get raised from inside getattr are going to be inscrutable. The traceback will be uninformative, and you'll be forced to drop into a debugger to figure out what's going on.

I started to agree with that but then I realized: I don't think you can get getattr to raise an error here at all. The keys are strings, the default value is provided, I don't think there's a fault path here.

In general though, I totally agree with you. I use less-cute code shamelessly when it will aid debugging. I write for the debugger. ;-)

> At runtime, your second example is actually significantly less explicit because so much is hidden away in mutable state.

Yeah, I see what you mean. You don't know which "F" or "reductor" triggered the error because it doesn't show up in the traceback because each assignment doesn't have its own line in the code. That's a good point. I've been there, and it sucks.

In this specific case I would actually most likely write out the three if.. else.. chain. If there were four or more I would use the gadget style. It's right on the borderline, and I would suspect that there won't be a fourth option in the future (although usually that would be a bad assumption; in this case I speculate the the object model isn't going to change any time soon.)

>I started to agree with that but then I realized: I don't think you can get getattr to raise an error here at all. The keys are strings, the default value is provided, I don't think there's a fault path here.

Of course you can ;) __getattr__ could invoke a network request that fails for all you know.

But yes this was more a general comment on this layout. The data-driven layout, while very explicit in code, is actually awful when you encounter issues at runtime, for exactly the reasons you describe.

While I'm not a fan of this PEP's syntax, I will say that I do think it someone helps here, it reduces the boilerplate from 3-4 lines in some cases to a single line, which in practice makes this more visually declarative, and keeps tracebacks sane.

That said this pattern is rare enough for me that I don't think I'll be using this tool anytime soon.

> Of course you can ;) __getattr__ could invoke a network request that fails for all you know.

OMG! Yes, of course, but at that point your traceback has bigger problems. :-)

This is the kind of thing I might turn into a simple function with short circuiting returns. I suppose the new syntax is just as readable, but I kind of like it this way a bit better.

    def get_reductor_value(cls, x):
        reductor = dispatch_table.get(cls)
        if reductor:
            return reductor(x)

        reductor = getattr(x, "__reduce_ex__", None)
        if reductor:
            return reductor(4)

        reductor = getattr(x, "__reduce__", None)
        if reductor:
            return reductor()

        raise Error("un(shallow)copyable object of type %s" % cls)
The new version is a little shorter (not important), without "technical" lines with reductor assignments and with the important getattr() expressions on the if lines (subtle and subjective, but somewhat important), and consistently indented with similar things at the same level (quite important).

All this for the small cost of learning a few simple scoping rules and the negligible cost of occasionally discovering the special cases in which the new syntax cannot be used.