Re: quotearg: implement custom_quoting_style

[Joel E. Denny]

On Sun, 23 Aug 2009, Bruno Haible wrote:

> Hi Joel,

Hi Bruno.

> > +2009-08-22  Joel E. Denny  <address@hidden>
> > +
> > +   quotearg-tests: test escaping of embedded locale quotes
> > +   * tests/test-quotearg.c (struct result_strings): Add member for
> > +   new input.
> > +   (LQ_ENC, RQ_ENC, RQ_ESC): New macros.
> > +   (inputs): Add new input.
> > +   (results_g): Add expected results.
> > +   (flag_results): Likewise.
> > +   (locale_results): Likewise.
> > +   (compare_strings): Check those.
> 
> Thanks. It's good to see the testsuite coverage increase.
> 
> >  2009-08-22  Joel E. Denny  <address@hidden>
> >  
> > +   quotearg: fix right quote escaping when it's in quote_these_too
> > +   * lib/quotearg.c (quotearg_buffer_restyled): Upon seeing a right
> > +   quote, be sure to prepend only one backslash.
> > +   * tests/test-quotearg.c (use_quote_double_quotes): New function.
> > +   (main): Test it.
> 
> Thanks as well. We were not aware of this bug.

Thanks.  Should I go ahead and push those?

> >  2009-08-22  Joel E. Denny  <address@hidden>
> >  
> > +   quotearg: implement custom_quoting_style
> > +   * lib/quotearg.c: (struct quoting_options): Add left and right
> > +   fields to store quotes.
> > +   (set_custom_quoting): New public function.
> > +   (quotearg_buffer_restyled): Add left and right arguments,
> > +   handle them very much like locale quoting, and update all uses.
> > +   (quotearg_n_custom): New public function.
> > +   (quotearg_n_custom_mem): New public function.
> > +   (quotearg_custom): New public function.
> > +   (quotearg_custom_mem): New public function.
> > +   * lib/quotearg.h: Prototype and document new public functions.
> > +   (enum quoting_style): Add custom_quoting_style member and
> > +   document.
> > +   * tests/test-quotearg.c (custom_quotes): New array.
> > +   (custom_results): New array.
> > +   (main): Extend to test custom quoting.
> 
> Looks good as well. I agree that the previously available choices for
> the quote strings - either hardcoded, or from the translated message catalog -
> are not sufficient for all use-cases.

Thanks for the prompt review.

> Just two points:
> 
>   - Can you please avoid 1-letter identifiers as function parameters?
>     'l' and 'r', in particular. At the first glance, I thought it would
>     be possible to set l = "[(" and r = "])".

Why isn't that possible?

  quotearg_custom ("[(", "])", "[(joel's])\tstring");

produces

  "[([(joel's\\])\\tstring])"

Is that not what you expect, or did I miss your point?

>     Can you call them
>     'left_quote' and 'right_quote', respectively? That would avoid this
>     misunderstanding.

Regardless of the above rationale, I agree with the better names.

>   - The 'if (quoting_style != custom_quoting_style) {' code should be
>     moved to before the big 'TRANSLATORS:' comment. You know that
>     this comment is meant to be extracted by xgettext, and xgettext
>     extracts it only if there is no other non-empty line between the
>     comment and the gettext() invocation.

I really need to study internationalization more.  I'll fix it.  Thanks.

> > +    /* Like c_quoting_style except use the custom quotation marks set by
> > +       set_custom_quoting.
> 
> This should be "Like clocale_quoting_style", not "Like c_quoting_style":
> Because c_quoting_style also handles trigraphs, which custom_quoting_style
> doesn't.

I missed that.  I copied those comments from the comments on 
clocale_quoting_style, which don't mention the trigraph distinction.  
I'll fix that too.

> >  2009-08-22  Joel E. Denny  <address@hidden>
> >  
> > +   quotearg: document limitations of quote_these_too
> > +   * lib/quotearg.c (quotearg_buffer_restyled): Add comments where
> > +   those limitations are created.
> > +   * lib/quotearg.h (set_char_quoting): Document that digits and
> > +   letters that are special after backslash are not permitted.
> > +   (quotearg_char): Cross-reference set_char_quoting documentation.
> > +
> 
> OK, good.

I'll wait to push this until after the custom_quoting_style patch.