[Bioperl-l] For CVS developers - potentialpitfallwith"returnundef"

[Chris Fields]

> -----Original Message-----
> From: bioperl-l-bounces at lists.open-bio.org [mailto:bioperl-l-
> bounces at lists.open-bio.org] On Behalf Of Sendu Bala
> Sent: Thursday, June 01, 2006 2:59 AM
> To: bioperl-l at lists.open-bio.org
> Subject: Re: [Bioperl-l] For CVS developers -
> potentialpitfallwith"returnundef"
> 
> Chris Fields wrote:
> >
> > Sendu Bala wrote:
> >> Just looking for all return undef;s isn't enough. It's entirely
> possible
> >> to do something like:
> >>
> >> my $return_value;
> >> {
> >>    # do something that assigns to return_value on success
> >>    # on failure, just do nothing
> >> }
> >> return $return_value;
> >
> > Agreed, though looking for these is obviously much harder.
> >
> > The way to get around those is:
> >
> > return $return_value if $return_value;
> > return;
> >
> > which I've seen used in a number of get/set methods.
> 
> Though if anyone is using that cookie-cutter/macro style, that's much
> worse because now you can't return 0.
> 
> return $return_value if defined($return_value);
> return;

Makes sense.  Really, this all comes down to semantics and the context of
how the method is called and what is expected as a return value.  I suppose
it also depends on what one considers 'best practice,' which can be
subjective.  I don't want us getting into a situation in which we come
across as critiquing someone else's code w/o some valid points, i.e.
Lincoln's point about complaining.  I think that's why this thread is pretty
important, in that we're getting a broad range of opinions on the issue.

> In any case, it burns the eyes. 

Yep, I agree. 

> I share Lincoln's POV. I also fully
> understand your point about not being able to trust the docs
> (Bio::Map::Marker...). But the solution is to change the code so they
> match the docs when the docs make sense, not change the code so that it
> no longer matches the docs[*]. In a massive OO project like bioperl the

So you know, Lincoln and I both support the idea of an audit.  He also notes
(and I agree) that people will likely complain.  

Anyway, changing the code to match the docs makes sense therotically, but in
practice that doesn't always work.  Any situation where code does not behave
as expected (i.e. as described in the docs) are bugs and can be reported as
such.  The problem arises when the docs are completely wrong, as
Bio::Restriction::IO was before I made changes to it.  In many cases simple
small code changes won't work, such as when methods inherit from an
interface but don't implement all methods (so essentially are incomplete).

Hilmar made the point that we should change the docs to reflect
inconsistencies in particular plugin modules for IO classes (AlignIO has a
few modules with unimplemented write methods, and so on).  When the code
radically varies, such as in the Restriction::IO case (where none of the
write methods worked), the docs should be changed in the IO class to reflect
this.  Of course, you should also add a bit to the TO DO section of POD and
add a bit to the Project Priority List on the wiki to point this out, both
of whichI did.  It comes down to 'truth in advertising', does it do what's
expected.

> users need to be able to rely on the docs. You can't turn around and say
> "you've used this method for years, but now I'm changing how it works
> because you might have used the method incorrectly". Ideally any code

Not what I did, BTW.  The API is intact; you can still use the write methods
if you want (they throw errors just fine).  In fact, I didn't change any
methods except in one module (Restriction::IO::bairoch), where I added a
warning to the read method b/c it didn't work as expected, and I filed a bug
report.  Essentially, the only thing I changed was the docs to reflect what
the code currently can accomplish (at least until you read the TO DO).  We
already had one person email the group asking why code in the synopsis
didn't work.

Adding read and write methods to most of these modules (making the code do
what the docs reflect, in your words) is a lot of work, esp. for someone
like me unfamiliar with the class architecture and methods for those
modules.  IMHO, contributions to bioperl should accomplish what is reflected
in their docs once added to the core; if a write method hasn't been written,
then add it to the docs in a TO DO section or add a warning to the synopsis.
Don't put in the docs what you intend the code to accomplish down the road
but what it does currently.  Is that unreasonable?

Anyway, when something doesn't perform as expected (produces invalid output
or contains errors), it's considered a bug.  That includes misrepresenting
what a module does in the docs.  When we try to fix bugs we have to decipher
what the intent of the original author was from the docs and code, then try
to get it to work by modifying the code.  In extreme cases (such as
unimplemented methods) that may mean writing up entire methods from scratch.
The read and write methods for IO modules are normally the longest methods
in a class.  That's a heck of a lot of effort for something that a large
majority of us aren't interested in taking up, esp. when the submitting
author should have had everything up to spec (i.e. what's in the docs) when
adding it to the core.

> changes add functionality or improve it's working without affecting code
>   that uses the method correctly according to its old docs.
> 
> 
> * though if there isn't time/interest in changing the code, and the
> method never worked as per the docs, then by all means change the docs
> to avoid confusion - just don't change the docs on a method that worked
> according to the docs, because then you can assume people use the method
> and will be affected by the change

Again, didn't do that.  The methods in the docs either didn't exist (not
implemented) or didn't work (contained bugs).  The docs were changed b/c
they were misleading.

-chris
 _______________________________________________
> Bioperl-l mailing list
> Bioperl-l at lists.open-bio.org
> http://lists.open-bio.org/mailman/listinfo/bioperl-l