On Wed, 2011-06-15, Stefan Fuhrmann wrote:
> On 15.06.2011 16:45, Daniel Shahaf wrote:
> > Julian Foad wrote on Wed, Jun 15, 2011 at 15:14:53 +0100:
> >> New in 1.7 in svn_types.h:
> >> svn_tristate_t
> >> svn_tristate_to_word() # yields "true"/"false"/NULL
> >> svn_tristate_from_word() # takes "true"/"yes"/"on"/.../
> >>
> >> The _to/from_word() functions don't seem canonically purposed: some
> >> users want a single representation of the values, for use in on-the-wire
> >> protocols and XML output, while other users want a more flexible,
> >> human-readable interpretation that accepts multiple different words
> >> ("true" = "on" = "yes").
> Well, these look like reasonable conversion utilities to me.
> They simply don't make assumptions about the context that
> they might be used in.
As *implementations* they work fine, serving both single-representation
and human-friendly purposes for their current callers. But the
*interface* definitions are "hinky" as Greg put it, or in my words "not
"canonically purposed": they are designed to serve two or more logically
different purposes. That's bad design.
As the software develops over time, for the human-friendly purpose it
would be perfectly reasonable for us to want to change the set of
accepted words from time to time, maybe adding "1" and "0" as recognized
inputs or changing the default output to "yes" and "no" instead of
"true" and "false". Or localize the human language used for input and
output. Or return some readable string instead of NULL for the
"unknown" value. Or even remove "true" and "false" from the set of
recognized inputs. (Yes, that would mean changing the config file
specification and the command-line option specification, and dealing
with associated back-compat issues.)
For the single-representation purposes (used for XML output and
server-client protocol), we would probably never change the text values
used, and if we ever did we would need to co-ordinate that change as
part of a protocol change, and we would never want this to happen as a
side effect of changing the human-friendly parsing of command-line
options.
For single-representation purposes, _from_word() serves adequately as a
complement to _to_word(), accepting "true", "false" and NULL. But for
human-friendly purposes, _to_word() does not serve as a clear logical
complement to _from_word(). It does not provide *the* way to convert a
tristate value to a human-readable string, it just provides *one* way
which some callers might want, and other callers might want a different
way. And it outputs NULL whereas some callers might well want a
human-readable word such as "unknown" or "default".
These are some of the reasons why, even if the interfaces for these
different purposes share some implementation, the interfaces should be
separate.
> However, one might argue whether they need to be public.
> Before making them private, we need to extend svn_cmdline
> to do the conversion. Otherwise, a svn tool (svnserve in that
> case) would need to call private API.
OK, thanks for the heads-up on that point.
> >> The doc string of _from_word() is wrong: it says any input other than
> >> "true" and "false" is unknown.
> Yep. Forgot to update that.
> >> The current users are:
> >>
> >> * The main use is in svn_log_changed_path2_t.text_modified
> >> and .props_modified. For these, we use _to_word() in mod_dav_svn to
> >> write a protocol string (assumes the value is not svn_tristate_unknown),
> >> use _from_word() in ra_neon and ra_serf to interpret a protocol string,
> >> and use _to_word() in "svn log --xml" to output an XML attribute, using
> >> an undocumented feature of svn_xml_make_open_tag() to allow the
> >> attribute string to be NULL. All of these need only "true"/"false", not
> >> yes/no/off/on.
> But they won't be hurt by allowing those alternative values.
Correct.
> >> * implementation of svn_config_get_bool() and svn_hash_get_bool(),
> >> which uses _tristate_from_word() to flexibly read "on"/"off"/"yes"/etc.
> The is where the more flexible implementation comes from.
> It looked a little duplicated.
Reducing duplication is good, yes. But not if the API suffers.
> >> * svnserve:main.c which uses it simply to detect whether a
> >> command-line option is "true"/"yes"/"on", and doesn't actually care
> >> about the tri-state-ness.
> >
> Well, not specifying the parameter is equivalent to the 3rd state.
> But this check actually prompted me *not* to duplicate the
> comparison code again but to factor it out to some common
> utility function. Even 3rd party coders might find it helpful.
Maybe parsing of command-line option boolean values and the config-file
boolean values should be considered logically the same; that makes sense
to me. Great - these two purposes can share a parser.
Note that neither of these two purposes currently wants an output
function, although it is conceivable that at some time they may do, in
order to print out help or examples (of command-line usage or config
file settings). If and when we do want an output function for these
purposes, is it certain that we would want the function to output
"true"/"false"/NULL? No, I don't think so. For example, for the
"unknown" state we might well want it to output a default value string
that is provided as an input parameter.
> >> At least, and at first, we should remove the second and third usages
> >> listed above.
> And replace them by what?
Replace the use of svn_tristate_from_word() by an equivalent but more
local API.
> >> Those usages are more akin to "svn_cmdline" or
> >> "svn_config" utilities, or could well be private.
> >>
> >> Further, I consider moving/renaming these two functions to be a bit more
> >> private even for their main use in svn_log_changed_path2_t.
> > Fair enough. I don't see that any of the above uses require third
> > parties API consumers to parse tristates (from string to enum), so no
> > objection to making _from_word() private.
> Making them private would be fine with me but that requires
> svn_cmdline to be extended with some equivalent public function.
Great, that's what I'll do then.
Thanks.
- Julian
Received on 2011-06-17 13:06:06 CEST