Best practices for naming classes in different modules meant for the same container

[mwchase]

I was looking at punq's source code, and I noticed that, when multiple classes have the same name, the behavior of register() is order-sensitive.

Specifically, if two modules define a helper class or alias with the same name, then the contents of one module are registered, then the other, then the client classes from the first module, then they will inject an instance of the second module's class instead of the first's.

If this is a concern for me, should I be using punq, and, if so, what's the recommended way to avoid collisions without cross-checking every module?

[bobthemighty]

Hi @mwchase that sounds like a bug, right? I'm happy to take a look and see if we can fix that for you.

I guess we should make sure to use a fully qualified name when relying on the type as a key.

[mwchase]

I was hesitant to ask for changes because every way I've come up with to address this is either implementation-specific, or a breaking change.

What's happening to trigger this bug is, the Registry object maintains a _localns dict, and that uses the unqualified name of the class. So, if two different classes have the same unqualified name, whichever one was registered most recently will be selected by get_type_hints.

(I don't think I checked whether this needs from __future__ import annotations. It definitely happens with it, and I think it might be needed.)

Anyway, my concern about breaking changes is, how should string keys be resolved? If you use the string name of a key, then either that needs to be fully-qualified, or punq has to inspect the globals to determine the fully-qualified name, and accessing the globals is either implementation-specific, or a new argument that needs to be added.

I tried fixing this on my own, and everything I tried made at least two tests fail.

I haven't been thinking about this for too long, so I hope I'm missing something.

[bobthemighty]

@mwchase I think that if you choose to use a string key, then it's on you to ensure that key is unique. If you provide a type as a key, then we should make sure that we use the fully qualified name to select the right thing.

If that causes a breaking change, I'm happy enough to introduce it with 0.5

[mwchase]

Okay, I just checked this a bit more, and it happens when the annotations on __init__ use string keys, which happens implicitly when using from __future__ import annotations.

I've got an idea of how I want to address this for my use case, but it's fiddly enough that I almost think I should fork punq to try it out, because I think my ideas here are too opinionated.

(The basic idea I have is to consider types where the string key doesn't match the type name as "aliases", and treat those as string keys, and otherwise convert it to the type.)

[mwchase]

Last night, I put together a set of tests demonstrating the issue: https://github.com/bobthemighty/punq/compare/master...mwchase:name-collision

I think I was a little abstract in my explanations; this should be more concrete.

I think I know how I want to change registration, assuming I figure out how I want resolution to work, but I haven't figured that out yet.

[mwchase]

Now that I've had a chance to look and think things over, I think I was over-estimating the magnitude of the changes I want, from a design perspective.

Basically, what I want to have is:

• String arguments to register and resolve are fully-qualified names
• A level of granularity in distinguishing aliases, such that aliases to the same type are considered distinct, provided they have different names or are defined in different modules.
• Instead of keyword arguments, the resolve methods take an optional dict of overrides, where the keys can be fully-qualified names (which would usually contain periods) or classes.

I've also given some thought to generics that I think would mostly qualify as "a new feature".

I'm a little burnt out right now I think, but when I'm in better shape, I'll start opening PRs. Probably more than one, because I think I've got multiple distinct changes I want to make.

EDIT MUCH LATER: In the time since, I've had to revise my approach again, because PEP 563 should break it. I have no idea whether the state of punq in this area has changed in the last two years, so this bug should be presumed stale.

However, I've gotten myself curious now, so I'll take a look at the code and add a followup edit.

FOLLOWUP: As long as there's _localns, I'm pretty sure I'll run into issues, so I'm going to keep on messing with my fork.