Re: My first cut at the patch

Stephen Warren <[email protected]> writes:

> [ ... ]
>
> A few questions that spring to mind are below; just thinking out loud.

Thank you very much!


> Lloyd Zusman wrote:
>> +from types import *
>
> What's the benefit of this v.s. "import types"?

None.  I had used it during development of this patch, where I had been
comparing something to StringType.  But I don't use that any more, and
hence, this import statement can be deleted ... which I will do in the
next version of this patch.


>> +DefaultVarPat = re.compile('^[A-Z][A-Z0-9_]*$')
>
> Should this include lower-case letters too?

Well, I only want to export TMDA variables that are explicitly defined
in Defaults.py, not all the other stuff that I also get when doing a
var(Defaults).  Those TMDA variables all match DefaultVarPat.

Hmm ... actually, the '0-9' part of the pattern might be superfluous.  I
thought that I had seen numbers within one or more of the names of
variables defined in Defaults.py, but now I can't find any examples of
this.  I'll check again after my morning caffeine.


>> +                try:
>> +                    if v is None:
>> +                        del os.environ[k]
>> +                    else:
>> +                        os.environ[k] = str(v)
>> +                except:
>> +                    pass
>
> What's the rationale for ignoring exceptions here?

I only want to put variables whose values can be converted to strings
into the environment, and ignore the rest.  During development, there
were some cases where this enviroment manipulation failed, but that
might have been before I started matching the variable names via
DefaultVarPat, as discussed above.  Therefore, it may be that those
failures no longer are occurring.  I'll double-check that, and if the
exception block isn't needed any more, I'll get rid of it.


>> +        Util.pipecmd('%s %s' % (hook, pendpath), self.show())
>
> Are there any issues that require quoting of these strings to avoid
> nasty shell escape issues? For some reason, I thought TMDA external
> commands were executed with a list of arguments, rather than a
> shell-style string that required parsing?

The Util.pipecmd method is already used in this manner in Deliver.py and
Util.py.  However, I'll look into this in more detail to see if I should
write my own alternative to Util.pipecmd that might be safer for use
with these new action hook functions.

Thanks again for all your feedback.



-- 
 Lloyd Zusman
 [email protected]
 God bless you.