Add Formatter.pop(type)

[minrk]

I copied some of the logic from apptools (thanks @rkern).

The current behavior:

• lookup(instance) - return appropriate callback or a given object
• lookup_by_type(type_or_str) - return appropriate callback for a given type or 'mod.name' type string
• for_type(type_or_str) - behaves the same, only adding support for type strings for consistency
• pop(type_or_str[, default]) - remove a type (by type or string)
• typ_or_str in Formatter checks whether a type is registered, checking with lookup_by_type

The inner structures remain unchanged.

[takluyver]

I don't know how many people are using this API, but if we were concerned about backwards compatibility, but didn't want the rather unintuitive reversal, we could None for querying, and have a separate sentinel for removal.

[ellisonbg]

Why not create a new set of remove methods?

On Wed, Nov 20, 2013 at 11:41 AM, Thomas Kluyver
notifications@github.comwrote:

I don't know how many people are using this API, but if we were concerned
about backwards compatibility, but didn't want the rather unintuitive
reversal, we could None for querying, and have a separate sentinel for
removal.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4561#issuecomment-28922438
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[takluyver]

Actually, yes, I think an explicit remove method is preferable.

[minrk]

What is the benefit of remove_for_type over type_printers.pop?

[minrk]

Does nobody else find it surprising that registering None as a formatter via for_type(type, None) doesn't clear the formatter?

[takluyver]

Mildly surprising, perhaps, but not enough to make me keen on an API-breaking change. Especially because the failure mode if people are using that to check the current formatter is quite annoying - it will still return the value they expect, but then stop using that function, with no warning.

To change my position again: type_printers.pop seems adequate.

[minrk]

I should also note that the changed behavior was entirely undocumented, and a search on GitHub found no evidence of users of the undocumented behavior.

[ellisonbg]

I am fine with pop.

[rkern]

FWIW: the API originates in the upstream pretty.py on which the type-dispatch was modelled:

http://dev.pocoo.org/hg/sandbox/file/98ce1ce17c7c/pretty/pretty.py#l634

[minrk]

Thanks, @rkern, makes more sense now.

Any preferences between:

1. no change, just publicize pop as official API
2. add remove_for_type
3. interpret None formatter as unregistration

[rkern]

I have a preference for adding remove_for_type() and query() methods and making for_type(typ, None) an error (after deprecation, if you like). Ideally, remove_for_type() and query() would be implemented to check both type_printers and deferred_printers since the deferred printers will get moved over to the type_printers cache when they are resolved. You can't do that if a raw type_printers.pop()/deferred_printers.pop() is your API.

It's too bad that I stuck you guys with separate methods for registering by type and type name. In my more recent incarnations of this pattern, I have put both in the same method. That makes it less of a load on the API to add removal and query methods.

https://github.com/enthought/apptools/blob/master/apptools/type_registry/type_registry.py

[ivanov]

Just a note, please add a docs/source/whatsnew/pr/formatter.rst to document this change, regardless of what we go with.

[minrk]

Okay, I copied some of the logic from apptools, thanks @rkern.

The current behavior:

• lookup(instance) - return appropriate callback or a given object
• lookup_by_type(type_or_str) - return appropriate callback for a given type or 'mod.name' type string
• for_type(type_or_str) - behaves the same, only adding support for type strings for consistency
• pop(type_or_str) - remove a type (by type or string)

The inner data structures are unchanged, so people reaching in and calling pop already should be unaffected.

@ivanov I will make sure to add the what's new notes, and also propagate usage of this API in the few places we would have used them if they existed.

[minrk]

what's new added, and pop usage propagated. Should be ready for another review pass.

[rkern]

The docstring says it returns a string, but it actually returns a tuple.

[minrk]

Thanks, sorry - that's my bad copying, I will take another run at the docstrings to find any other things that don't map.

[minrk]

The lookup, pop behavior is directly from apptools, but I was considering making the following changes:

• lookup_by_type doesn't support type strings, unlike for_type and pop. I think it should, for consistency.
• I want to add a dict.pop(typ, None) behavior like dict.pop to prevent KeyError
• map __contains__ to lookup_by_type, so one can check typ in Formatter

Any objections to those?

[rkern]

1. I can't think of an actual use case for a string typename for lookup_by_type(). YAGNI, but it does no harm.
2. Good thought.
3. __contains__ should return a bool, IMO.

[minrk]

1. I can't think of an actual use case for a string typename for lookup_by_type().

I was mainly thinking of wanting to register a formatter for a not-yet-imported type, but only if one doesn't already exist:

if 'not_yet_imported.type' not in formatter:
    formatter.for_type('not_yet_imported.type', my_formatter)

Also, it was just for consistency, since I kept trying to use it this way in the tests to verify that things were working properly, and all of the other API calls support it.

contains should return a bool, IMO.

yes, definitely. I only meant to use lookup_for_type to perform the check, the return is still bool.

The code should reflect these changes now.

[rkern]

The docstrings could use another pass (for instance, rationalizing the type description of typ across all of the methods), but that shouldn't hold up merging.

[Carreau]

I didn't saw anything specific to say, but this is a place of the code where I'm not confortable.