[pygr-notify] Issue 66 in pygr: Code review: seqdb.py

[codesite-noreply at google.com]

Comment #3 on issue 66 by the.good.doctor.is.in: Code review: seqdb.py
http://code.google.com/p/pygr/issues/detail?id=66

See

http://groups.google.com/group/pygr-dev/browse_thread/thread/f03601c835c6bc57/1425a52a7f67c33d?lnk=gst&q=prefixuniondict#1425a52a7f67c33d


where Chris says:

"""
I'll answer the PrefixUnionDict questions in a separate email.
"""

There are a few unanswered questions in there still, I think: to quote,

SeqPrefixUnionDict is oddly named.

PrefixUnionDict could contain __iadd__ behavior (instead of
SeqPrefixUnionDict, which inherits from it solely to add __iadd__
behavior).  Why not do things this way?

There's an 'if' statement in SeqPrefixUnionDict.__iadd__ that can
never be reached, as far as I can tell.  Could someone (Chris) please
verify that for me?

In two places (SeqPrefixUnionDict.__iadd__, find @CTB untested; and
_PrefixUnionDictInverse.__getitem__, find @CTB abstraction) explicit
knowledge of annotation objects is assumed in classes that otherwise
don't know anything special about them.  Any way to deal with this
better or should I just suck up and deal & write some tests?

A few PrefixUnionDict issues:

  - PrefixUnionDict.__contains__ can take either a sequence ID or a
    sequence object.  This is inconsistent with __getitem__ (which only
    takes seq IDs).  Ugly.

  - PrefixUnionDict.__init__ takes an argument 'trypath' that is really a
    list of paths to try.  Could we rename this to 'trypaths'?  Or should
    I just suck up and deal & write a nice doc?

  - PrefixUnionDict.writeHeaderFile function (etc.) isn't used anywhere
    in the code.  How useful is it?

  - PrefixUnionDict.newMemberDict(...) and the associated
    _PrefixUnionDictMember class don't have a clear purpose to me.
    Could someone give me a concrete use case?  If not, maybe they should
    be removed or something; they're confusing!

  - _PrefixUnionDictMember.default handling could probably be written
    in a more standard way using dict.setdefault, no?  How much code depends
    on this specific behavior?

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings