Mattias Engdegård wrote on Tue, May 07, 2013 at 00:45:44 +0200:
> 6 maj 2013 kl. 23.39 skrev Daniel Shahaf:
>
> >The patch achieves its end in an unmaintainable way. It's liable to
> >break when APR's implementation details change, and since it
> >ultimately
> >attempts to parse something which wasn't intended to be parsed, it
> >might
> >do the _wrong_ thing (as opposed to just not doing anything) someday.
>
> No, APR is constrained by its API; it cannot change arbitrarily.
>
Aware of that, but "%s: %s: %c\n" is not an API promise, it's an
implementation detail.
> >I can't see today what other circumstances APR would call errfn->("%s:
> >%s: %c\n", _, "invalid option", _) in, but they don't guarantee us
> >that
> >there won't be other circumstances so I tend not to assume that they
> >won't. That's how I treat Subversion APIs that I write and call, and
> >that's how I treat APR too: I rely on promises, not on implementation
> >details.
>
> No, the patch will translate "%s: invalid option: %s\n" and a few
> other messages as specified by the .po files. There is no way it can
> "do the wrong thing".
>
Of course there is, if someday APR uses "invalid option" in another
meaning than the one this patch assumes it does.
I already said, I don't see exactly how that can happen since errfn has
very few and specific usages --- but I don't want to assume no one will
ever find another creative use for errfn that your patch does not
anticipate.
That's why API promises exist: so anticipating won't be needed. The API
promises something and you work to that. The API doesn't promise you
something and you don't presume it'll happen.
> The worst that can happen is that someone changes the spelling or
> formatting of one of the messages in APR. The consequence is then
> that this message will be displayed untranslated, and that the
> translators may not immediately get to know about it since the
> changed strings are not seen by the gettext string extractor.
> Clearly, this is no worse than the present situation.
>
> Yes, this patch relies on promises made by the APR - the errfn
> signature and semantics.
>
No, not the signature, but the specific arguments and their meaning.
> Should APR eventually contain a more sophisticated error-reporting
> interface in the future, the proposed code could of course be
> retired in favour of a use of that interface. Until then, the patch
> would serve its purpose.
>
Well yeah, it could grow an enum.
Or it could just promise that "invalid option" in the 2nd replaceable
argument will always be okay to translate. That doesn't require any
code changes, just a decision by the APR devs not to introduce other
meanings to that particular string in the future.
> >>Since the patch:
> >>1) solves an annoying problem,
> >
> >Which no one complained about.
>
> Two translators have pointed out the defect. Either should be
> sufficient.
>
I've missed the previous report of that problem. (Is that why Jens is
on the CC? I haven't seen him in a while.)
> >Keep in mind that error message in the libraries are translated, and
> >'svn help' is translated. Is it really that important to translate
> >'invalid option'? If it is, wouldn't someone have complained about it
> >on users@?
>
> Surely the worthiness of bugs to be fixed isn't measured in the
> number of complaints to the users@ mailing list?
The facts are that two translators complained and no user complained.
It's not a question of "5 users v. 5000 users".
> Of course it's important to translate that message -- if
> localisation is important at all, that is.
>
> >Feel free to point me at another place in svn's code that ignores API
> >non-promises or parses something that wasn't intended to be parsed.
>
> The suggested patch does not misuse the API. Should APR change,
> crashes or corruption cannot occur; the API guarantees that.
That's correct, it won't crash. It might print a different message than
APR intended.
Received on 2013-05-07 01:01:00 CEST