Make metadata.__contains__ accept Table objects
(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)
-
repo owner -
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.
-
repo owner - changed milestone to 0.6.xx
-
Account Deleted (original author: ged) IMO, this should be decided before 0.6, not 0.6.xx, because it breaks the API.
-
Account Deleted (original author: ged) at least could break...
-
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. -
repo owner OK I would most prefer
__contains__
to only acceptTable
objects here, not keys, so the change would be completely backwards incompatible. I doubt many projects if any besides elixir are using this feature. -
repo owner - changed status to resolved
in a burst of holiday cheer I have it converting incoming
Table
objects toTable.key
, which is the proper string keyname, so both forms are supported. 0d2ae16aee7d99aeb89d3aa087df6d693900b093 -
repo owner - removed milestone
Removing milestone: 0.6.0 (automated comment)
- Log in to comment
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 aTable
...).