search

isi-vista / immutablecollections

A library for immutable collections, in the spirit of Guava's Immutable Collections.
MIT License
3 stars 2 forks source link

Runtime type checking on immutablecollections #1

Closed gabbard closed 5 years ago

gabbard commented 5 years ago

Issue by joelb-git Monday Nov 27, 2017 at 19:03 GMT Originally opened as https://github.com/isi-nlp/isi-flexnlp/issues/93

In the new data model, we'd like to have type checking of theory items that contain (refer to) other theory items, e.g.

Old model:

        e = Entity(doc, 'ORG', [123])
TypeError: expected: <class 'flexnlp.model.Mention'>, got: <class 'int'>

New model:

        e = Entity(EntityType('ORG'), [123])

No error here.

In general, we want (runtime) type checking on all immutablecollections.

gabbard commented 5 years ago

Comment by ConstantineLignos Tuesday Nov 28, 2017 at 15:12 GMT

@rgabbard Can you take a stab at this? I'm imagining an additional argument to the immutable collection of methods that specify whether to check isinstance of each thing in the collection and raise a TypeError if it doesn't match. Then update attrutils attrib_{opt}_immutable.

It's probably easiest if you branch off of immutable-document to something like immutable-document-features to add this and whatever other things you add.

gabbard commented 5 years ago

Comment by ConstantineLignos Tuesday Nov 28, 2017 at 16:00 GMT

While you're touching immutable collections, see if you can clean up this idiom:

self._regions_to_include: ImmutableSet[str] = (ImmutableSet.of(regions_to_include)
                                    if regions_to_include is not None
                                    else ImmutableSet.empty())

attrib_opt_immutable may already have the solution, I can't remember how that works.

gabbard commented 5 years ago

Comment by rgabbard Wednesday Nov 29, 2017 at 23:29 GMT

@ConstantineLignos : the second case can be slightly improved (?) to

self._regions_to_include: ImmutableSet[str] = (ImmutableSet.of(regions_to_include or ImmutableSet.empty()))

Which is still awkward. If performance isn't super-important, ImmutableSet.of(regions_to_include or []) is not bad.

gabbard commented 5 years ago

Comment by rgabbard Wednesday Nov 29, 2017 at 23:31 GMT

Ah, wait, does ImmutableSet.empty() magically evaluate to false like a regular empty collection?

gabbard commented 5 years ago

Comment by ConstantineLignos Wednesday Nov 29, 2017 at 23:33 GMT

I haven't tested but I'm pretty sure AbstractSet[T] will take care of that.

gabbard commented 5 years ago

Comment by rgabbard Friday Dec 08, 2017 at 15:21 GMT

This is done for ImmutableSet (optionally, should it be on by default?) but not ImmutableList, ImmutableDict, or ImmutableMultiDict

gabbard commented 5 years ago

I believe we have determine that the performance cost of this is significant enough we do not want to pursue it as part of this library.