Bert Huijben wrote:
>> @@ -2225,8 +2228,36 @@ svn_client_blame(const char *path_or_url
>> * @note @a relative_to_dir doesn't affect the path index generated by
>> * external diff programs.
>> *
>> + * @since New in 1.6.
>> + */
>> +svn_error_t *
>> +svn_client_diff5(const apr_array_header_t *diff_options,
>> + const char *path1,
>> + const svn_opt_revision_t *revision1,
>> + const char *path2,
>> + const svn_opt_revision_t *revision2,
>> + const char *relative_to_dir,
>> + svn_depth_t depth,
>> + svn_boolean_t ignore_ancestry,
>> + svn_boolean_t no_diff_deleted,
>> + svn_boolean_t ignore_content_type,
>> + svn_boolean_t ignore_mergeinfo,
>> + const char *header_encoding,
>> + apr_file_t *outfile,
>> + apr_file_t *errfile,
>> + const apr_array_header_t *changelists,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *pool);
>
> This method has 5 different versions over 7 subversion point releases, each adding just on or a few 'optional' parameters.
>
> Shouldn't we create some kind of context/args object that we can extend in future versions for methods that probably have yet another version in the next release?
>
> Adding boolean number 4 to a function doesn't make user code more understandable, and with a context/args struct with a constructor function we can just extend that structure in the next release without breaking the ABI.
I thought about this same issue when extending the status API on the
ignore-mergeinfo branch. I suspect there are several APIs we could do this too
next time we rev them. If we identify those, we should put a note in their
comments.
Bert, could you write the patch (against the ignore-mergeinfo branch, of course)
for this?
Thanks,
-Hyrum
> We do use argument structures in a few places in the subversion code (think user context, status, authorization, ...), but I think diff would be a good candidate to follow this construction. And there are probably a few other places that could benefit from such argument structures.
>
> =====================
> In SharpSvn I use the convention of args objects for most optional parameters, so I don't have to create another override for every future version of subversion that adds just another boolean parameter like this.
>
> Using the old NSvn that followed the subversion arguments like most other bindings, you would use something like:
> [[
> Client.Diff("a", "b", diffStream, true, true, false, false, true);
> ]]
> Completely unreadable without intellisense or documentation open.
>
>
> While a SharpSvn user would write something like:
>
> [[
> SvnDiffArgs args = new SvnDiffArgs();
> args.IgnoreContentType = true;
> args.IgnoreMergeInfo = true;
>
> Client.Diff("a", "b", diffStream, args);
> ]]
>
> A little bit more code, but no need to set default values. I think everybody can understand it without looking at the documentation.
>
> (Using C# 3.0 instead of 2.0 would allow you to use a more compact syntax)
>
>
> Adding a new property to the args object does not break the ABI and does not require a new compatibility wrapper to maintain forever.
Received on 2008-09-17 15:36:26 CEST