Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn

Hi kou

On 8/10/07, Kouhei Sutou <kou@cozmixng.org> wrote:
> Hi,
>
> In <ae6cb1100708102119n1d6c5490n886cb1eb4ee944aa@mail.gmail.com>
> "Re: svn commit: r26006 - in trunk/subversion/bindings/swig: include ruby/libsvn_swig_ruby ruby/svn" on Fri, 10 Aug 2007 21:19:07 -0700,
> "Joe Swatosh" <joe.swatosh@gmail.com> wrote:
>
> > However, I'm still unhappy with the svn_swig_rb_conflict_resolver_func
> > function itself. It feels like it should return the
> > svn_wc_conflict_result_t, but I wasn't sure how to make that happen.
> > Not to mention that there is no test for any of this (I've no idea how
> > I could tie this together).
>
> I don't know about the recent API but test_resolved in
> test_client.rb will help you. Because the test causes
> conflict on purpose.

I'll peek at that to see if I can come up with anything.

>
> > Now that you've let me get started complaining, I'm not nuts about all
> > the default arguments to Svn::Wc::AdmAccess#update_editor and
> > switch_editor. Is it time to start considering a hash? How about (DO
> > NOT COMMIT!):
>
> > It looks like we've broken compatibility with 1.4.x for that function
> > already. That may have to be a different discussion.
>
> I think it's not good to break API compatibility for now. I
> think the time is 2.0.0. In my thought, we define xxx_yyy2
> methods for hash argument version. And xxx_yyy methods still
> use the current style. How about this idea?

That seems reasonable. If I can scare up the time, I'll have a try at
that. At some point we'll have to check to ensure that we haven't
changed the apis. I just did a diff between the 1.4.x branch and
trunk and I think update_editor has a new required argument (perhaps
just a rename, though?).

>
> Of cause we should validate arguments of hash argument
> version methods (xxx_yyy2). We'll need to use the following
> method:
>
> class Hash
> def assert_valid_keys(*valid_keys)
> unknown_keys = keys - [valid_keys].flatten
> unless unknown_keys.empty?
> raise(ArgumentError, "Unknown key(s): #{unknown_keys.join(", ")}")
> end
> end
> end
>
> # The above method is quoted from ActiveSupport.
>

I'm not familiar with this. Is it just to ensure that there were no
typos in the passed in options? I was thinking it might be nice just
to ignore unrecognized options. I haven't given it a lot of thought,
but it seemed like clients might be able to create their own options
hashes, and their might be room for reuse if we ignored unrecognized
keys.

Thanks for listening,

--
Joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org