On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote: > -1. I think it's very useful to have routines for this sort of thing > that return an error message rather than emitting an error report > directly. That gives the caller a lot more control.
Please let me counter-argue here. There are a couple of reasons to not do as the patch proposes: 1) Consistency with the error messages makes less work for translators, who already have a lot to deal with. The patch is awkward in this sense, to give some examples: + if (s != PG_STRTOINT_OK) { - pg_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("invalid status interval: %s", parse_error); } [...] - pg_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("invalid compression level: %s", parse_error); 2) A centralized error message can provide the same level of details. Here are suggestions for each error status: pg_log_error("could not parse value for option %s", optname); pg_log_error("invalid value for option %s", optname); optname should be defined by the caller with strings like "-t/--timeout" or such. Then, if ranges are specified and the error is on a range, I think that we should just add a second error message to provide a hint to the user, if wanted by the caller of pg_strtoint64_range() so an extra argument could do handle that: pg_log_error("value must be in range %d..%d") 3) I think that we should not expose directly the status values of pg_strtoint_status in pg_strtoint64_range(), that's less for module authors to worry about, and that would be the same approach as we are using for the wrappers of pg_strto[u]intXX() in the patch of the other thread (see pg_strto[u]intXX_check for example in [1]). [1]: https://www.postgresql.org/message-id/20190910030525.ga22...@paquier.xyz -- Michael
signature.asc
Description: PGP signature