bug-gnustep
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: NSSavePanel complains about .hidden


From: Nicola Pero
Subject: Re: NSSavePanel complains about .hidden
Date: Mon, 7 Oct 2002 00:09:00 +0100 (BST)

Ok - it looks silly to discuss too much about this anyway (with all the
pending stuff) but I can't avoid another one on this :-)

> >I don't understand why you are assuming that +stringWithContentsOfFile
> >returning nil is an error.  The caller will assess that.  In the case of
> >NSSavePanel, for example, it's a perfectly valid condition.
> >
> 
> <pedantic>Technically a nil return here indicates a failure ... an 
> error.  I'ts up to the caller to decide how to deal with that, not to 
> decide whether it's a failure or not.</pedantic>

<pedantic>An 'error', for the programmer using the base library, is when
the base library generates an exception.  Anything else is not an error.  
A nil return here indicates that the file doesn't exist, is not readable
or can't be read.  This is considered a valid result, otherwise the API
would have required an exception - with the name and reason of the error -
to be raised.</pedantic>

But I agree that it's up to the caller to decide how to deal with a nil
result, which is why the library should not be logging but letting the
caller decide what to do. :-)

Yes - it's a very simple (but not very powerful) API ... the idea I
suppose is that if you want richer control, then you have to use a more
complex API.

+stringWithContentsOfFile: is a simple high-level wrapper around the
low-level API, which provides you with a simple and straightforward API
for reading a file if it's there, and ignore it if it's not or can't be
read.  It's perfect for cases like the NSSavePanel, which need precisely
that.  The fact that it returns nil is to be considered a *feature* of the
library, and using it is *not* a programming error.

If you don't like the API, because you think failing to read the file in
this method is an error, and the API should be raising an exception with
the exact name and reason of the error, that's your own personal
disagreement/dislike of the API.  If you don't like it, don't use it then
:-) - why don't you use more low-level APIs to check that the file exists,
read the contents, etc checking each step, if you want more control ?

But I wouldn't build in the library hardcoded logging to stderr which
can't be turned off, just because you feel the simpler API does not
provide information about what caused a nil result.  Whatever the
solution, it must be a good/modern one :-) not a primitive hardcoded
uncustomizable log.


> >This is why the warning should be a NSDebugLog - off by default (as it is
> >an undocumented and possibly unwanted warning about a 'maybe error', not
> >an actual official error), and should only be turned on if you require it
> >by using the appropriate --GNU-Debug stuff.
> 
> The warnings are 'warn' logs ... you can control whether they are compiled in 
> to the
> library or not.

Yes - but while the debug logs can be turned off by the user/programmer at
run time, the warn logs can't be controlled at run time.

I suppose if they are to stay as NSWarnLog, then I'd vote for warn=no to
become the default in gnustep-make (maybe warn=yes could still be turned
on by debug=yes).


> The purpose of a warn log is to highlight things that the progrmmer has done 
> which
> we think are probably wrong

Good explanation - but then why the following code -

  tmp = NSZoneMalloc(zone, fileLength);
  if (tmp == 0)
    {
      NSWarnFLog(@"Malloc failed for file (%s) of length %d - %s",
        thePath, fileLength, GSLastErrorStr(errno));
      CloseHandle(fh);
      return NO;
    }

is this an error that the programmer has done when calling the method ?

The explanation of why you are using NSWarnLog is not very convincing.  
You are checking the result of each operation you do, and producing a
NSWarnLog is any operation fails.  But it's not the programmer's fault if
memory can't be allocated, or if a file was deleted by another process
just after the method was called (but before the file was opened), or its
permissions changed in the same way, or if the file is too big or if an
fseek on the file fails (or if the NFS went down just after the method
started executing).

This stuff is not in the programmer's control, and not the programmers's
fault, and a different programming style would not necessarily remove the
cause of the errors - so it should not be a NSWarnLog.  Why are you
against it being a NSDebugLog ?


By the way, it seems you want to always have the calling code separately
check if the the file is there (and that it's not a dir), then check if
the file is readable by you, and only then read it using
+stringWithContentsOfFile:.

Well in the NSSavePanel I don't see how that would help or be a better
programming style ... it would require us to write a lot more code in
NSSavePanel to do exactly the same things - read the file if it's there
and can be read as a file, and ignore the matter in all other cases.

It would also be less efficient, since it would check twice that the file
is readable (basically part of the +stringWithContentsOfFile: would need
to be duplicated in the caller, which *is* bad programming style and cause
of bugs).


> (not cases where we are 100% sure ... in those cases we
> would have an NSLog()

I'd say you should never have a NSLog() unless it's a base library
installation/version/internal problem, potentially preventing the library
from working.


> or raise an NSException)

much better :-)


> and provide them with guidance to improve their code, as well as 
> providing more technically savvy users with some diagnostic aids.
> 
> The fact that both Adam and I independently chose the same mode of logging 
> here
> indicates to me that we probably both think that attempting to create an 
> object from
> a file in order to determine if the file exists or is readable (when there is 
> an API for
> doing exactly those tests) is bad programming practice,

If there is an API for doing exactly those tests, you call the API if you
want to do the tests.  If you don't want to do the tests, and just want to
get the string if the file exists and nil if the file doesn't, then you
call the API which will do just that, return the string if the file exists
and can be read, and nil otherwise. :-)



> and that therefore in most
> cases where an attempt to read from a file fails, there has been a programming
> error of some sort,

There are millions of reasons why a read from a file can fail without any
programming error - no more memory, NFS server down, NFS network problems,
file removed, permissions changed, cdrom/floppy unmounted, filesystem
error, harddisk failure, floppy failure, cdrom damaged ... any of these
can happen at any time; and conditions can very well change between the
moment that the programmer checks that the file is readable and the moment
that he reads it.  Not so uncommon.

I can see that some people might want a NSDebugLog of what's happening to
help in debugging weird problems.  But it has nothing to do with
programming errors or programming practice or teaching people how to
improve/fix their code.


> and we ought to let the programmer know so they can improve/fix their code.

The NSSavePanel code is correct, is using the documented API, exactly as
documented, and efficiently and elegantly - I see no reason to change it.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]