Re: OperatingSystem.py

Tim Hemel wrote:

> On Sun, Aug 31, 2003 at 11:18:18PM +0200, Bram Moolenaar wrote:
> > 
> > A proposal for refactoring is welcome.  Aap has gotten to the point that
> > most functionality is present and we can overview what's there, so that
> > some cleaning up can be done,
> 
> Well, here is one. It is a first attempt at getting the os.name code smell
> out. It is far from perfect, but it could clean things up a little bit.

Thanks.  Seeing the code organised this way I spot a few things that
need to be improved.  That means it's a good idea to do it.  I'll make a
few specific remarks below.

> By the way, be sure to check the Python cookbook, it has got a lot of
> scripts, including the win32 peculiarities.

Problem with these things is that with Python it's often a lot quicker
to write the code yourself than to search around for existing code...

> Prepare for more...

We at least need the diffs for the places that use the new class!

> ---OperatingSystem.py----------------------------------------------
> import os
> 
> # suggestions:
> #
> # for shorten_name, use relative path script:
> # http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/208993
> #
> # for dir_contents, use os.listdir instead of glob

Is there a specific reason to use os.listdir() instead of glob()?

> # Util.py, line 1075: /usr/local/share/aap should be changed to whatever is
> # used at install time.

Some systems may prefer to use /etc/aap or /opt/aap.  Perhaps the path
can be set in default.aap, so that it can be changed when installing
that file.  Or we should use a separate configuration recipe that is
generated when installing.

> class OperatingSystem:
>     def quoteShell(self,str):
>         """Util.py line 422"""
>         pass    

For each function there should be a comment that defines what it does.
In this case it needs to mention what kind of quoting is to be done.
This actually is very complicated (how do you quote a command that
contains a quote?  On MS-Windows I have found this to be nearly
impossible.).

>     def system(self,cmd):
>         """Util.py line 835"""
>         # default implementation
> 
>         # system() on MS-Windows can handle only one command at a time and
>         # must not end in a NL.  Do the same on other non-Unix systems for
>         # now.
>         # TODO: system() isn't available on the Mac

Before Mac OS X.  I tend to say Aap is only supported on Mac OS X.  If
someone wants to make it work on OS 9, be my guest.  I don't think it is
worth the effort, more and more people are using OS X.

>         # TODO: system() always returns zero for Windows 9x

This is a serious problem, because errors go unnoticed.  Does that
cookbook has a solution for this?

[..]

>     def canUseTee(self):
>         """this should be named differently, perhaps isNotInteractive?

This is specifically used to decide whether to invoke "tee".  I don't
see another use for it.

>         # but perhaps it is better to abstract the code that is used after
>         # this...

This is one of those cases where moving the code is doubtful.  You might
end up implementing redir_system() in the operating system class, which
is probably not a good idea.

>     def getFullFname(self,name):

Please use a space after a comma, that makes it much easier to read.

>     def systemAsync(self,cmd):
>         if not os.fork():
>             n = os.system(cmd)
>             os._exit(n)

This is quite inefficient, forking the whole Python interpreter and not
doing an "exec" right away...  Can't be solved easely, trying to avoid
using it is probably the best solution.

> class WindowsOperatingSystem(OperatingSystem):
>     def quoteShell(self,str):
>         # On MS-Windows double quotes works well
>         return double_quote(str)

A few things I learned from Vim is that it's nearly impossible to
execute a command that has a double quote in it.  And the method used
for redirecting depends on the shell used (put the > inside the quotes
or outside the quotes?).  And some people use Cygwin bash, which breaks
all of it...  Hopefully this is something we can use from another
project.  SCons might have a solution.

>     def getExtList(self):
>         return ['', '.exe', '.com', '.bat', '.cmd' ]

Perhaps we should add ".pif" and ".scr"?  You can also execute ".py" if
the settings are done for it.  That is getting too complicated, we might
have to leave it as it as and add a remark in the documentation.

> > It's very simple:  Run "aap" without arguments in the aap/Exec
> > directory.
> 
> That is indeed so simple that I would have never thought of it.

This might be a good opportunity to add tests for those things that are
changed.  Joerg Beyer has created a start for the unittests, we should
be adding more tests now!

-- 
       His head smashed in,  and his heart cut out,
       And his liver removed, and his bowels unplugged,
       And his nostrils raped, and his bottom burned off,
       And his penis split ... and his ...
                 "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

 /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net   \\\
///          Creator of Vim - Vi IMproved -- http://www.Vim.org          \\\
\\\              Project leader for A-A-P -- http://www.A-A-P.org        ///
 \\\  Help AIDS victims, buy here: http://ICCF-Holland.org/click1.html  ///


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf