> On 19 Nov 2024, at 11:20, Masahiro Ikeda <ikeda...@oss.nttdata.com> wrote:
> I've tested it and reviewed the patch. I'd like to provide some feedback.
Thank you very much for your review and I do apologize for the late response.
> I tested with v3 patch and found the following compile error.
> It seems that math.h needs to be included in variables.c.
>
> variables.c: In function 'ParseVariableDouble':
> variables.c:220:54: error: 'HUGE_VAL' undeclared (first use in this function)
> 220 | (dblval == 0.0 || dblval >= HUGE_VAL ||
> dblval <= -HUGE_VAL))
> | ^~~~~~~~
Odd, I don't see that, but I've nonetheless fixed it by including math.h
> Although the error handling logic is being discussed now, I think it would be
> better,
> at least, to align the logic and messages of exec_command_watch() and
> ParseVariableDouble().
I see your point, especially since ParseVariableBuffer is only used for this at
the moment. It is however intended as generic functionality and would require
to diverge from the other ParseVariableXXX to support custom error messages.
I'm not sure it's worth the added complexity for something which is likely to
be a quite niche feature.
> ParseVariableDouble() doesn't have a comment yet, though you may be planning
> to add one later.
Fixed.
> I believe the default value is 2 after the WATCH_INTERVAL is specified
> because \unset
> WATCH_INTERVAL sets it to '2'. So, why not update the following sentence
> accordingly?
>
> - of rows. Wait the specified number of seconds (default 2) between
> executions.
> - For backwards compatibility,
> + of rows. Wait the specified number of seconds (default 2, which can
> be
> + changed with the variable
> + between executions. For backwards compatibility,
>
> For example,
>
>> Wait <varname>WATCH_INTERVAL</varname> seconds unless the interval option is
>> specified.
>> If the interval option is specified, wait the specified number of seconds
>> instead.
>
> + This variable sets the default interval which
> <command>\watch</command>
> + waits between executing the query. Specifying an interval in the
> + command overrides this variable.
>
>> This variable sets the interval in seconds that <command>\watch</command>
>> waits
>> between executions. The default value is 2.0.
I don't think this improves the documentation. The patch only changes the
defult interval which is used if the interval is omitted from the command, the
above sections discuss how interval is handled when specified. I did some
minor tweaks to the docs to try and clarify things.
> I think it's better to replace queries with executions because the \watch
> documentation says so.
>
> + HELP0(" WATCH_INTERVAL\n"
> + " number of seconds \\watch waits between queries\n");
Fair point, fixed.
The attached v4 fixes the above and also adds tests.
--
Daniel Gustafsson
v4-0001-Make-default-watch-interval-configurable.patch
Description: Binary data
