You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Max Bowsher <ma...@ukf.net> on 2005/02/17 12:04:51 UTC
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros subversion/bindings/swig subversion/bindings/swig/ruby subversion/bindings/swig/ruby/libsvn_swig_ruby subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext subversion/bindings/
I have a number of comments:
> Log:
> Merge Ruby SWIG-based bindings patch.
> http://pub.cozmixng.org/~kou/diff/svn-add-ruby-20050216.diff
>
...
> * subversion/bindings/swig/core.i:
> added SWIG/Ruby related codes.
> swap order of %include and #include.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No, you didn't do this. So don't include it in the log message! Ditto for
all the other occurrences of it.
> * subversion/bindings/swig/ruby/test/my-assertions.rb: [NEW]
> original assertions for test.
>
> * subversion/bindings/swig/ruby/test/util.rb: [NEW] utility
> methods for test.
>
> * subversion/bindings/swig/ruby/test/test_client.rb: [NEW]
> tests for svn/client.rb.
>
> * subversion/bindings/swig/ruby/test/run-test.rb: [NEW]
> test running script.
>
> * subversion/bindings/swig/ruby/test/test_core.rb: [NEW]
> tests for svn/core.rb.
>
> * subversion/bindings/swig/ruby/test/test_fs.rb: [NEW]
> tests for svn/fs.rb.
>
> * subversion/bindings/swig/ruby/test/test_repos.rb: [NEW]
> tests for svn/repos.rb.
>
> * subversion/bindings/swig/ruby/test/test_delta.rb: [NEW]
> tests for svn/delta.rb.
>
> * subversion/bindings/swig/ruby/test/test_info.rb: [NEW]
> tests for svn/info.rb.
>
> * subversion/bindings/swig/ruby/test/test_util.rb: [NEW]
> tests for svn/util.rb.
>
> * subversion/bindings/swig/ruby/svn/util.rb: [NEW]
> utilities for svn/*.rb.
We don't have a standard of using [NEW] - instead we just say "New file." In
fact that is *all* you need to say - no need to restate what is obvious from
the file name anyway.
> * subversion/bindings/swig/ruby/svn/ext/*.rb: [NEW]
> dummy for svn/ext/_*.so.
Do not use wildcards! (this is documented in HACKING)
Also, why are these present for only three of the _*.so modules?
> * subversion/bindings/swig/ruby/svn/client.rb: [NEW]
> wrapper of svn/ext/_client.so for easy to use.
Not a big deal, but "for easy to use" doesn't actually make sense. "for ease
of use" would be correct, but as mentioned above, a simple "New file" would
have been fine.
Also, please use a capital letter at the beginning of a sentence or phrase.
...
> Modified: branches/ruby/subversion/bindings/swig/svn_client.i
> Url:
> http://svn.collab.net/viewcvs/svn/branches/ruby/subversion/bindings/swig/svn_client.i?view=diff&rev=13034&p1=branches/ruby/subversion/bindings/swig/svn_client.i&r1=13033&p2=branches/ruby/subversion/bindings/swig/svn_client.i&r2=13034
> ==============================================================================
> --- branches/ruby/subversion/bindings/swig/svn_client.i (original) +++
> branches/ruby/subversion/bindings/swig/svn_client.i Tue Feb 15 21:24:45
> 2005
...
> @@ -280,6 +322,45 @@
> argvi++;
> }
>
> +%runtime %{
> + #include <apr.h>
> + #include <apr_pools.h>
> +
> + static apr_pool_t *
> + _svn_client_pool(void)
> + {
> + static apr_pool_t *__svn_client_pool = NULL;
> + if (!__svn_client_pool) {
> + apr_pool_create(&__svn_client_pool, NULL);
> + }
> + return __svn_client_pool;
> + }
> +
> + static apr_pool_t *
> + _svn_client_config_pool(void)
> + {
> + static apr_pool_t *__svn_client_config_pool = NULL;
> + if (!__svn_client_config_pool) {
> + apr_pool_create(&__svn_client_config_pool, _svn_client_pool());
> + }
> + return __svn_client_config_pool;
> + }
> +%}
This will affect all languages, not just Ruby!!!
> Modified: branches/ruby/subversion/bindings/swig/svn_delta.i
> Url:
> http://svn.collab.net/viewcvs/svn/branches/ruby/subversion/bindings/swig/svn_delta.i?view=diff&rev=13034&p1=branches/ruby/subversion/bindings/swig/svn_delta.i&r1=13033&p2=branches/ruby/subversion/bindings/swig/svn_delta.i&r2=13034
> ==============================================================================
> --- branches/ruby/subversion/bindings/swig/svn_delta.i (original) +++
> branches/ruby/subversion/bindings/swig/svn_delta.i Tue Feb 15 21:24:45
> 2005
> @@ -82,6 +89,7 @@
> #include "swigutil_rb.h"
> #endif
> %}
> +%include svn_delta.h
>
> %include svn_delta.h
>
Bad merge!!!
> Modified: branches/ruby/subversion/bindings/swig/svn_ra.i
> Url:
> http://svn.collab.net/viewcvs/svn/branches/ruby/subversion/bindings/swig/svn_ra.i?view=diff&rev=13034&p1=branches/ruby/subversion/bindings/swig/svn_ra.i&r1=13033&p2=branches/ruby/subversion/bindings/swig/svn_ra.i&r2=13034
> ==============================================================================
> --- branches/ruby/subversion/bindings/swig/svn_ra.i (original) +++
> branches/ruby/subversion/bindings/swig/svn_ra.i Tue Feb 15 21:24:45 2005
> @@
> -115,9 +115,16 @@ #include "swigutil_rb.h"
> #endif
> %}
> +%include svn_ra.h
>
> %include svn_ra.h
>
Bad merge!!!
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros
subversion/bindings/swig subversion/bindings/swig/ruby
subversion/bindings/swig/ruby/libsvn_swig_ruby
subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext
subversion/bindings/
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,
In <06...@robinson.cam.ac.uk>
"Max Bowsher" <ma...@ukf.net> wrote:
> > I can hack to remove svn/ext/*.rb. Is it a better way?
>
> Yes - I've just committed a change, have a look at it, tell me what you
> think.
It seems to be working well.
Thanks.
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros subversion/bindings/swig subversion/bindings/swig/ruby subversion/bindings/swig/ruby/libsvn_swig_ruby subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext subversion/bindings/
Posted by Max Bowsher <ma...@ukf.net>.
Kouhei Sutou wrote:
> Hi,
>
> In <00...@robinson.cam.ac.uk>
> "Max Bowsher" <ma...@ukf.net> wrote:
>
>>>>> * subversion/bindings/swig/ruby/svn/ext/*.rb: [NEW]
>>>>> dummy for svn/ext/_*.so.
>>>>
>>>> Also, why are these present for only three of the _*.so modules?
>>>
>>> User should not require svn/ext/_*.so directory. They are
>>> only required internally in
>>> svn_{client,fs,ra,repos,wc}.c. Because
>>> svn_{client,fs,ra,repos,wc}.i have some of `%import
>>> svn_{delta,fs,wc}.i'.
>>
>> But why do svn/ext/*.rb exist? Python and Perl don't require extra files
>> because of these %import-s.
>
> In Ruby bindings, Svn::Ext::* (Svn::Ext::Client,
> Svn::Ext::Core and so on) are API which is generated by SWIG
> and Svn::* (Svn::Client, Svn::Core and so on) are API which
> is made by me for useful. In Svn::Ext::* layer, we don't
> know Svn::*. So svn/ext/_*.so `require "svn/ext/*"'. But I
> want svn/ext/_*.so to require svn/*.rb.
>
> svn/ext/*.rb are just wrappers for requiring svn/*.rb. For
> example:
>
> In svn/ext/_client.so: require "svn/ext/delta"
> The code requires svn/ext/delta.rb.
> ->
> In svn/ext/delta.rb: require "svn/delta"
> The code requires svn/delta.rb.
> ->
> In svn/delta.rb: require other svn/*.rb and define useful
> API.
>
>
> I can hack to remove svn/ext/*.rb. Is it a better way?
Yes - I've just committed a change, have a look at it, tell me what you
think.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros
subversion/bindings/swig subversion/bindings/swig/ruby
subversion/bindings/swig/ruby/libsvn_swig_ruby
subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext
subversion/bindings/
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,
In <00...@robinson.cam.ac.uk>
"Max Bowsher" <ma...@ukf.net> wrote:
> >>> * subversion/bindings/swig/ruby/svn/ext/*.rb: [NEW]
> >>> dummy for svn/ext/_*.so.
> >>
> >> Also, why are these present for only three of the _*.so modules?
> >
> > User should not require svn/ext/_*.so directory. They are
> > only required internally in
> > svn_{client,fs,ra,repos,wc}.c. Because
> > svn_{client,fs,ra,repos,wc}.i have some of `%import
> > svn_{delta,fs,wc}.i'.
>
> But why do svn/ext/*.rb exist? Python and Perl don't require extra files
> because of these %import-s.
In Ruby bindings, Svn::Ext::* (Svn::Ext::Client,
Svn::Ext::Core and so on) are API which is generated by SWIG
and Svn::* (Svn::Client, Svn::Core and so on) are API which
is made by me for useful. In Svn::Ext::* layer, we don't
know Svn::*. So svn/ext/_*.so `require "svn/ext/*"'. But I
want svn/ext/_*.so to require svn/*.rb.
svn/ext/*.rb are just wrappers for requiring svn/*.rb. For
example:
In svn/ext/_client.so: require "svn/ext/delta"
The code requires svn/ext/delta.rb.
->
In svn/ext/delta.rb: require "svn/delta"
The code requires svn/delta.rb.
->
In svn/delta.rb: require other svn/*.rb and define useful
API.
I can hack to remove svn/ext/*.rb. Is it a better way?
Regards,
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros subversion/bindings/swig subversion/bindings/swig/ruby subversion/bindings/swig/ruby/libsvn_swig_ruby subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext subversion/bindings/
Posted by Max Bowsher <ma...@ukf.net>.
Kouhei Sutou wrote:
> "Max Bowsher"
> <ma...@ukf.net> wrote:
>
>>> * subversion/bindings/swig/ruby/svn/ext/*.rb: [NEW]
>>> dummy for svn/ext/_*.so.
>>
>> Also, why are these present for only three of the _*.so modules?
>
> User should not require svn/ext/_*.so directory. They are
> only required internally in
> svn_{client,fs,ra,repos,wc}.c. Because
> svn_{client,fs,ra,repos,wc}.i have some of `%import
> svn_{delta,fs,wc}.i'.
But why do svn/ext/*.rb exist? Python and Perl don't require extra files
because of these %import-s.
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13034 - in branches/ruby: . build/ac-macros
subversion/bindings/swig subversion/bindings/swig/ruby
subversion/bindings/swig/ruby/libsvn_swig_ruby
subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext
subversion/bindings/
Posted by Kouhei Sutou <ko...@cozmixng.org>.
Hi,
Thank you for many comments and fixing them.
In <00...@robinson.cam.ac.uk>
"Re: svn commit: r13034 - in branches/ruby: . build/ac-macros subversion/bindings/swig subversion/bindings/swig/ruby subversion/bindings/swig/ruby/libsvn_swig_ruby subversion/bindings/swig/ruby/svn subversion/bindings/swig/ruby/svn/ext subversion/bindings/" on Thu, 17 Feb 2005 12:04:51 -0000,
"Max Bowsher" <ma...@ukf.net> wrote:
> > * subversion/bindings/swig/ruby/svn/ext/*.rb: [NEW]
> > dummy for svn/ext/_*.so.
>
> Also, why are these present for only three of the _*.so modules?
User should not require svn/ext/_*.so directory. They are
only required internally in
svn_{client,fs,ra,repos,wc}.c. Because
svn_{client,fs,ra,repos,wc}.i have some of `%import
svn_{delta,fs,wc}.i'.
--
kou
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org