Re: string function - "format" function proposal

On Thu, Sep 9, 2010 at 8:57 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I am sending a updated version.
>
> changes:
> * tag %v removed from format function,
> * proprietary tags %lq a iq removed from sprintf
> * code cleaned
>
> patch divided to two parts - format function and stringfunc (contains
> sprintf function and substitute function)

=== Discussions about the spec ===
Two patches add format() into the core, and substitute() and sprintf() into
stringfunc contrib module. But will we have 3 versions of string formatters?

IMHO, substitute() is the best choice that we will have in the core because
functionalities in format() and sprintf() can be achieved by combination of
substitute() and quote_nullable(), quote_ident(), or to_char(). I think the
core will provide only simple and non-overlapped features. Users can write
wrapper functions by themselves if they think the description is redundant.

=== format.diff ===
* It has a reject in doc, but the hunk can be fixed easily.   1 out of 2 hunks FAILED -- saving rejects to file
doc/src/sgml/func.sgml.rejCOMMENT: We have the function list in alphabetical order, so format() should be inserted
afterencode().
 
* It can be built without compile warnings.
* Enough documentation and regression tests are included.

=== stringfunc.diff ===
* It can be applied cleanly and built without compile warnings.
* Documentation is included, but not enough. COMMENT: According to existing docs, function list are described with
<variablelist>or <table>.
 
* Enough regression tests are included.
* COMMENT: stringfunc directory should be added to contrib/Makefile.

* BUG: stringfunc_substitute_nv() calls text_format(). I think we don't need stringfunc_substitute_nv at all. It can be
replacedby stringfunc_substitute(). _nv version is only required if it is in the core because of sanity regression
test.

* BUG?: The doc says sprintf() doesn't support length modifiers, but it is actually supported in broken state:
postgres=# SELECT sprintf('%*s', 2, 'ABC');sprintf
---------ABC      <= should be ERROR if unsupported, or AB if supported.
(1 row)

* BUG?: ereport(ERROR,        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),         errmsg("unsupported tag \"%%%c\"",
tag)));Isthe code ok if the tag (a char) is a partial byte of multi-byte character?My machine prints ? in the case, but
itmight be platform-dependent.
 

=== Both patches ===
* Performance: I don't think those functions are not performance-critical, but we could cache typoutput functions in
fn_extraif needed. record_out would be a reference.
 

* Coding: Whitespace and tabs are mixed in some places. They are not so important because we will run pgindent, but
carefulchoice will be preferred even of a patch.
 

-- 
Itagaki Takahiro