Make metadata.__contains__ accept Table objects

[Former user]

(original reporter: ged) I just wrote something like "assert a_table in metadata" in my code and was disappointed to see it fail because I think it makes sense. Here is a patch to implement it. Since I'm not sure it fits your "vision", I didn't commit it directly.

[Mike Bayer]

I like it, I'm just not sure if its considered good form for __contains__() to accept two different kinds of values like that (like, I'd almost opt for it to only accept a Table...).

[Former user]

(original author: ged) In all honesty, I don't think it is considered good form. But that might be a case of "practicality beats purity". FWIW, I usually think more of MetaData as a set of tables than a pure mapping of names to tables (even though the mapping is of course useful). The question here is which collection interface you want to implement. I think that whatever interface you choose should be implemented more completely (and reading the TODO in the class, it seems like you agree with me on that point).

It might make sense to implement the set interface at the class level, on top of the .tables mapping. For the contains method, that would mean to accept only Table objects at the MetaData level, and let people who want to check by name use the .tables attribute explicitly: "a_table_name" in metadata.tables

The problem with this approach is that it would probably break a lot of code for only a marginal gain.

[Former user]

(original author: ged) IMO, this should be decided before 0.6, not 0.6.xx, because it breaks the API.

[Former user]

(original author: ged) at least could break...

[Mike Bayer]

well I'm leaning towards adding a method of some kind. I think __contains__() is just a bad target here in general.

[Mike Bayer]

OK I would most prefer __contains__ to only accept Table objects here, not keys, so the change would be completely backwards incompatible. I doubt many projects if any besides elixir are using this feature.

[Mike Bayer]

in a burst of holiday cheer I have it converting incoming Table objects to Table.key, which is the proper string keyname, so both forms are supported. 0d2ae16aee7d99aeb89d3aa087df6d693900b093

[Mike Bayer]

Removing milestone: 0.6.0 (automated comment)