I haven't tried the code, but on inspection I'd be surprised if simply doing this will return undef:
$schema->_error("Foo"); return;
Why not ?
That should be return undef; not just return.
the method is defined to return a single scalar. if you use return; and say someone writes
some_sub('foo', $schema->parse("bar"),1);
then the arguments passed will be wrong. return; should only be used when returning an empty list in a list context is the right thing todo.
However, I can appreciate that you are just copying what is already here :-)
The first line is simply a rewrite of the original statement that makes use of
the _error() method in Net::LDAP::Schema.
It has one side effect however: now parse() fails when used as a function on
an unblessed hash reference [i.e. parse(\%hash, "schema.ldif"); ]
So ?
Also, what happens when ->new("schema.ldif") fails to parse the schema in
the LDIF file? I *think* if parse returns undef then there's no way to
figure out where the problem in the LDIF file was. (A problem when there
are maybe a couple of thousand pieces of schema in the file :-)
IMHO that is a design question how the ->new(ARG) interface with argument
should behave:Should it return undef when parse(ARG) fails or should it
succeed even if parse fails.
The former is the current behaviour with my parse() patch while the latter can
be achieved with a minor change even if parse() is allowed to fail.
There is always a dilema of what todo when there is an error in a contructor. In the past I have always had it return undef, but then you need a way to flag the error.
For example in IO::Socket->new a failure sets $@ and returns undef, but this actually confuses a lot of people. there was some discussion recently on perl5-porters about creating new variable, a dualvar like $!, for this purpose.
I think if the constructor fails it should return undef. As a constructor that is given an arg can easily be written as
my $s = Net::LDAP::Schema->new
unless ($s->parse($arg)) {
...handle error..
}I don't think we really need to worry about how to report the error if it fails. If the programmer cares, they should use the long form.
So new should just be
sub new {
my $pkg = shift;
my $self = bless {}, $pkg;
@_ ? $self->parse(@_) : $self
}and parse should return $self on success and undef on error.
I can see the advantage of not failing i.e. knowing where the error was.
On the other hand I'd rather let if fail because I do not like objects to
exist when the initialization failed.
In addition to that Net::LDAP::Schema offers the possibilitiy to split the
->new(ARG) into ->new() and ->parse(ARG) [as Claude did]
So the former case mightbe considered a short cut with coarse error handling
while the latter offers full error handling.
I should read all the message before hitting reply :-)
Maybe we should let a higher authority (guess who) decide ;-)))
[ looks up ]
And since the old behaviour of ->new("0") failing even with existing legal
LDIF file "0" was not ideal too I changed it as well
good
Graham.
