[Bioperl-l] Feature/Annotation rollback finished

[Chris Fields]

On Aug 29, 2007, at 9:02 PM, Hilmar Lapp wrote:

>
> On Aug 28, 2007, at 5:05 PM, Chris Fields wrote:
>
>> I'm now wrapping up the Feature/Annotation rollback.  I will probably
>> start merging back to the main branch in the next day or two., as
>> soon as interested parties (*cough*devs*cough*) look over the last
>> batch of changes.
>>
>> http://www.bioperl.org/wiki/Feature_Annotation_rollback#Fourth_Round
>>
>> [...]
>> It is also possible there are still some instances where overloading
>> is expected lurking about in the ~1000 or so modules, so I'll leave
>> the exceptions I added to all Bio::AnnotationI
>
> Keep in mind that code such as
>
> 	if ($ann) { ... }
>
> is mostly not b/c someone wanted to use overloading, but rather
> someone was lazy and really meant to say
>
> 	if (defined($ann)) { ... }

Agreed.

> In the absence of eq overloading, these will behave identically. So
> if you leave the exceptions in it is sort-of policing lazy
> programmers, which I guess is fine in principle, but is guaranteed to
> trip up a lot of script code. I'd take it out if you're reasonably
> sure that at least within BioPerl itself those lazy programming
> incidents are removed.

I agree the overload exceptions shouldn't be left in.  The problem is  
I'm not certain we have caught most implicit overload calls (just the  
ones tested for).  Scott's checking everything against GMOD, though,  
so we can remove them after that.

>> [...]
>> The key change in this last round is the addition of several class
>> *dbxref* methods to Bio::Ontology::Term and
>> Bio::Annotation::OntologyTerm, all of which are capable of working
>> with either DBLink instances or simple scalars.
>
> I don't think you need the code here to deal with both scalars and
> objects. It is fine I think to define the new methods from the outset
> to consistently accept and return DBLink objects, and period.
>
> The backwards compatibility logic should rather be in the *_dblink*()
> methods; i.e., instead of simple aliases they should have the code to
> map to and from the new API. That way, once the deprecation cycle
> ends, they can be removed, and with them all the legacy code that now
> is no longer needed, whereas if you have that in the new methods, it
> keeps bothering the maintainers.

That should be easy enough to fix and would be more consistent.  I  
can look over the various calls to dbxref methods and see what needs  
to be done, then fix that in cvs.

> You also mention a add_dbxref_context() on the wiki page - I'm not
> sure why that would be needed given that you build in the -context
> option to add_dbxref() from the outset. But maybe I've glossed over
> some detail.

The -context parameter was in get_dbxref(), to grab those DBLinks in  
a particular context.  We could do the same with add_dbxref() (pass  
DBLinks in first arg as array ref, context as second arg).  That  
would then obviate the need for add_dbxref_context().

I'll also change the parameter passing in get_dbxref() to just accept  
context as an single optional argument since we're dealing with only  
DBLink instances now.

> Once this is merged back to the main trunk, I guess we need to give
> Bio::SeqFeature::TypedSeqFeatureI a thorough look and make sure it
> makes real sense.

It describes one method, ontology_term(), which returns a  
Bio::Ontology::TermI.  This is similar to SeqFeature::Annotated::type 
(), which returns a Bio::Annotation::OntologyTerm (a  
Bio::Ontology::TermI).  My thought is to simply deprecate type() in  
favor of TypedSeqFeatureI::ontology_term().

> Thanks Chris for this effort, this clears a monumental roadblock.
>
> 	-hilmar

No problem.  It just needed to be done.

chris