You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Berlin <db...@dberlin.org> on 2005/08/16 23:01:07 UTC
python-bindings-improvements review
So i reviewed the python-bindings-improvements branch.
I've assumed for the purposes of review that the swig code it generates
works (and did test it on my linux machine with the python scripts we
have). I have to leave it to others to verify this as well, on
platforms like win32, ruby, perl, etc.
I pointed out some moderate and minor things to David on IRC, like some
missing overview comments about what gen_swig is actually doing.
I don't see any major problems in the design or implementation, and in
fact, think it's a lot cleaner than what we do now.
However, since Branko seemed to be so against it, i thought i'd just
mention why i don't really agree that this is "spaghetti code".
First off, i should say that i don't agree with the idea that it
shouldn't t inherit from the Unix Makefile generator. Swig binding
generation really is like unix makefile generation, much moreso than
anything else. It So the fact that it inherits from the unix Makefile
generator really doesn't bother me that much.
The number of windows specific things in it is actually pretty small.
It's small enough that i think the cost of abstracting it and having a
design like:
gen_swig
/ \
/ \
/ \
/ \
/ \
/ \
/ \
gen_unix_swig gen_windows_swig
is not really worth it, and i doubt it will become worth it (you guys
can beat me up later if i turn out to be wrong :P).
There is probably some code that could be moved into the generator
utilities, etc. David has agreed to do so.
Other than that, assuming it is tested well by others, i believe it
should go in.
I'm not sure the procedure for large branch merges where it's unlikely
you can break it up into more than 2 or 3 patches.
I told David he should probably post the invariably huge patch against
head and let people pick at it a bit as a start, and we'll go from
there.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: python-bindings-improvements review
Posted by "C. Michael Pilato" <cm...@collab.net>.
Branko Čibej <br...@xbc.nu> writes:
> >Other than that, assuming it is tested well by others, i believe it
> >should go in.
> >
> I've not tested tnis on Windows yet, and I don't know if anyone
> has... lack of time on my part again, etc...
I've done some testing of various states of this code on Windows, but
I wouldn't call my work exhaustive by any stretch. I plan to do more
testing of such if/when time allows.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: python-bindings-improvements review
Posted by Branko Čibej <br...@xbc.nu>.
David James wrote:
>On 8/16/05, Branko Čibej <br...@xbc.nu> wrote:
>
>
>>Well, if we have
>>
>> gen_base --> gen_swig
>>
>>and some ifs in gen_swig, then fine. But
>>
>> gen_base --> gen_make --> gen_swig
>>
>>looks like a lie to me.
>>
>>
>gen_base is an abstract base class, so gen_swig needs to define
>"_extension_map" in order for it to be instantiated with gen_base as a
>parent. Right now, we can fulfill this requirement by inheriting from
>"gen_make", which defines "_extension_map". This is a temporary
>workaround until we refactor the generator classes.
>
>
Is that the only reason? Look at gen_win.py:GeneratorBase -- just copy
that to gen_swig.py, possibly changing the extensions to empty strings
to avoid confusion, and you're done.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: python-bindings-improvements review
Posted by David James <ja...@gmail.com>.
On 8/16/05, Branko Čibej <br...@xbc.nu> wrote:> Daniel Berlin wrote:> > >However, since Branko seemed to be so against it, i thought i'd just> >mention why i don't really agree that this is "spaghetti code".> >> >First off, i should say that i don't agree with the idea that it> >shouldn't t inherit from the Unix Makefile generator. Swig binding> >generation really is like unix makefile generation, much moreso than> >anything else. It So the fact that it inherits from the unix Makefile> >generator really doesn't bother me that much.> >> >> But it doesn't generate makefiles, right? So what's wrong with> inheriting from gen_base?[...]> Well, if we have> > gen_base --> gen_swig> > and some ifs in gen_swig, then fine. But> > gen_base --> gen_make --> gen_swig> > looks like a lie to me.
gen_base is an abstract base class, so gen_swig needs to define"_extension_map" in order for it to be instantiated with gen_base as aparent. Right now, we can fulfill this requirement by inheriting from"gen_make", which defines "_extension_map". This is a temporaryworkaround until we refactor the generator classes.
> >There is probably some code that could be moved into the generator> >utilities, etc. David has agreed to do so.> >> >> Yup. The whole generator thing could probably do with another round of> refactoriing, but that's not David's job. (Yet. :-p )Definitely! I'm thinking of creating three new helper modules: generator.util.external -- Calculating paths and options forexternal libraries generator.util.include -- Parsing include files and calculatinglists of include files generator.util.options -- Parsing build.conf and options passed onthe command-line
These helper modules will allow us to simplify the generator, andreduce interdependencies. With these helper modules, there'll be noneed for strange-looking inheritance trees (e.g., gen_swig depends ongen_make).
These changes can wait until after the merge, though.
> >Other than that, assuming it is tested well by others, i believe it> >should go in.> I've not tested tnis on Windows yet, and I don't know if anyone has...> lack of time on my part again, etc...C. Mike has done some preliminary testing on Windows.
> >I'm not sure the procedure for large branch merges where it's unlikely> >you can break it up into more than 2 or 3 patches.> >> >I told David he should probably post the invariably huge patch against> >head and let people pick at it a bit as a start, and we'll go from> >there.> >> >> I think a "svn diff -rFOO:BAR> http://svn.collab.net/repos/svn/branches/python...." is sufficient.> There's n point in posting something anyone can pull from the repository. :)It's nice to be able to look at the changes in your email reader, soyou can comment on the individual changes. I think there are somefolks who prefer this.
Cheers,
David
-- David James -- http://www.cs.toronto.edu/~james
Re: python-bindings-improvements review
Posted by Branko Čibej <br...@xbc.nu>.
Daniel Berlin wrote:
>However, since Branko seemed to be so against it, i thought i'd just
>mention why i don't really agree that this is "spaghetti code".
>
>First off, i should say that i don't agree with the idea that it
>shouldn't t inherit from the Unix Makefile generator. Swig binding
>generation really is like unix makefile generation, much moreso than
>anything else. It So the fact that it inherits from the unix Makefile
>generator really doesn't bother me that much.
>
>
But it doesn't generate makefiles, right? So what's wrong with
inheriting from gen_base?
>The number of windows specific things in it is actually pretty small.
>
>It's small enough that i think the cost of abstracting it and having a
>design like:
>
> gen_swig
> / \
> / \
> / \
> / \
> / \
> / \
> / \
>gen_unix_swig gen_windows_swig
>
>is not really worth it, and i doubt it will become worth it (you guys
>can beat me up later if i turn out to be wrong :P).
>
>
Well, if we have
gen_base --> gen_swig
and some ifs in gen_swig, then fine. But
gen_base --> gen_make --> gen_swig
looks like a lie to me.
>There is probably some code that could be moved into the generator
>utilities, etc. David has agreed to do so.
>
>
Yup. The whole generator thing could probably do with another round of
refactoriing, but that's not David's job. (Yet. :-p )
>Other than that, assuming it is tested well by others, i believe it
>should go in.
>
>
I've not tested tnis on Windows yet, and I don't know if anyone has...
lack of time on my part again, etc...
>I'm not sure the procedure for large branch merges where it's unlikely
>you can break it up into more than 2 or 3 patches.
>
>I told David he should probably post the invariably huge patch against
>head and let people pick at it a bit as a start, and we'll go from
>there.
>
>
I think a "svn diff -rFOO:BAR
http://svn.collab.net/repos/svn/branches/python...." is sufficient.
There's n point in posting something anyone can pull from the repository. :)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org