Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should trait_names return iterator or list? [P3 Compat] #240

Closed
rmorshea opened this issue Jun 3, 2016 · 8 comments
Closed

should trait_names return iterator or list? [P3 Compat] #240

rmorshea opened this issue Jun 3, 2016 · 8 comments
Milestone

Comments

@rmorshea
Copy link
Contributor

rmorshea commented Jun 3, 2016

HasTraits.trait_names and HasTraits.class_own_trait_names return a dict_keys iterator in Python 3, but a list in Python 2. Should this remain true, or should we be returning lists in both versions?

  • It would be in the style of Python 3 to keep the iterators
  • However, we could return lists for the sake of uniformity

As a side note, returning a dict_keys iterator prevents the actual dict object it refers to (which is never made available to the user) from being garbage collected. So if we want to keep iterators, that might be something to consider changing.

@minrk
Copy link
Member

minrk commented Jun 22, 2016

I think the current behavior - generator on Python 3, list on Python 2 makes the most sense to me. I don't think dict_keys would prevent it from being garbage collected unless someone is holding on to a reference to it, so that's probably what should be investigated, rather than using a different iterable object.

@rmorshea
Copy link
Contributor Author

In Py3 creating a dict_keys object increases the ref count.

from sys import getrefcount

d = {}

i = getrefcount(d)
k = d.keys()
f = getrefcount(d)

not_str = '' if i<f else '!'
msg = 'initial(%s) %s< final(%s)'
print(msg % (i, not_str, f))

@minrk
Copy link
Member

minrk commented Jun 24, 2016

Yes, but only as long as a reference is held to the iterator, because it is ultimately a reference to the dictionary itself. This would be true of any iterator that's not referring to a copy of the data. del k clears the reference, as would any exit from a closure referring to one.

@rmorshea
Copy link
Contributor Author

rmorshea commented Jun 24, 2016

I just felt it made more sense to copy the keys and return an iterator so the dict (which is never used) could be released. And not that it changes functionality, but I imagine it would be surprising for someone to find that the objection returned by trait_names() was a dict_keys object rather than a normal iterator, because the average user won't know it's derived from traits().

In any case I'm just nitpicking now since I'm no longer waiting on this.

@rmorshea
Copy link
Contributor Author

rmorshea commented Jun 24, 2016

An implementation without dict_keys would be something like this:

def trait_names(self, **metadata):
    for n in dir(self.__class__):
        v = getattr(self.__class__, n)
        if isinstance(v, TraitType):
            for meta_name, meta_eval in metadata.values():
                if type(meta_eval) is not types.FunctionType:
                    meta_eval = _SimpleTest(meta_eval)
                if not meta_eval(v.metadata.get(meta_name, None)):
                    break
            else:
                yield n

@minrk
Copy link
Member

minrk commented Jun 24, 2016

If a list would be simpler, I think it's fine for it to be a list.

@rmorshea
Copy link
Contributor Author

rmorshea commented Jun 24, 2016

If it were a list, and didn't depend on traits(), I would do as above (just appending to list instead of yielding). If it did depend on traits(), I'd just wrap the keys: return list(self.traits().keys()).

@minrk
Copy link
Member

minrk commented Jun 27, 2016

Depending on traits is fine, so you can do list(self.traits())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants