Make metadata.__contains__ accept Table objects

Issue #1541 resolved
Former user created an issue

(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.

Comments (9)

  1. Mike Bayer repo owner

    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...).

  2. Former user Account Deleted

    (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.

  3. Former user Account Deleted

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

  4. Mike Bayer repo owner
    • changed milestone to 0.6.0

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

  5. Mike Bayer repo owner

    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.

  6. Log in to comment