Some comments about v4:
Patch applies. "git apply" complained about a space or line somewhere, not
sure why. make check ok.
- rebased on post-psql hooks master
- took nearly every suggestion for change to documentation
Indeed. Looks ok to me.
- \if ERROR will throw the entire \if..\endif into IGNORE mode
Ok. I think that it is the better behavior, but other people opinion may
differ. Opinions are welcome.
- state is now pushed on all \ifs
- reinstated leveraging of ParseVariableBool
- history is now kept in interactive mode regardless of \if-truth
- reworked coding example to cause less agita
- removed MainLoop "are endifs balanced" variable in favor of in-place
check which respects ON_ERROR_STOP.
- make changes to psql/Makefile to enable TAP tests and created t/ directory
- wrote an intentionally failing TAP test to see if "make check" executes
it. it does not. need help.
A failing test expects code 3, not 0 as written in "t/001_if.pl". With
is($retcode,'3','Invalid \if respects ON_ERROR_STOP');
instead, the tests succeeds because psql returned 3.
More check should be done about stdout to check that it failed for the
expected reasons though. And maybe more tests could be added to trigger
all possible state transition errors (eg else after else, elif after else,
endif without if, if without endif, whatever...).
I'm hoping my failure in that last bit is easy to spot/fix, so I can move
forward with testing unbalanced branching, etc.
Other comments and suggestions:
Variable "slashCmdStatus" is set twice in 3 lines in "mainloop.c"?
There is a spurious empty line added at the very end of "mainloop.h":
#endif /* MAINLOOP_H */
I would suggest to add a short one line comment before each test to
explain what is being tested, like "-- test \elif execution", "-- test
Debatable suggestion about "psql_branch_empty": the function name somehow
suggests an "empty branch", where it is really testing that the stack is
empty. Maybe the function could be removed and "psql_branch_get_state(...)
== IF_STATE_NONE" used instead. Not sure.
"psql_branch_end_state": it is a pop, it could be named "psql_branch_pop"
which would be symmetrical to "psql_branch_push"? Or maybe push should be
named "begin_state" or "new_state"...
C style details: I would avoid non mandatory parentheses, eg:
+ return ((strcmp(cmd, "if") == 0 || \
+ strcmp(cmd, "elif") == 0 || \
+ strcmp(cmd, "else") == 0 || \
+ strcmp(cmd, "endif") == 0));
I would remove the external double parentheses (( ... )). Also I'm not
sure why there are end-of-line backslashes on the first instance, maybe a
macro turned into a function?
+ return ((s == IFSTATE_NONE) ||
+ (s == IFSTATE_TRUE) ||
+ (s == IFSTATE_ELSE_TRUE));
I would remove all parenthenses.
+ return (state->branch_stack == NULL);
Sent via pgsql-hackers mailing list (email@example.com)
To make changes to your subscription: