[PATCH 0/2] syntax-check: Introduce a rule for one line error messages
This is a follow up of: https://listman.redhat.com/archives/libvir-list/2023-August/241416.html And while the regular expression in patch 2/2 is very trivial it mostly works. But I'm open for suggestions. Michal Prívozník (2): tools: Reformat --help output of virsh and virt-admin syntax-check: Introduce a rule for one line error messages build-aux/syntax-check.mk | 8 ++++++++ tools/virsh.c | 6 ++++-- tools/virt-admin.c | 6 ++++-- 3 files changed, 16 insertions(+), 4 deletions(-) -- 2.41.0
The --help output of virsh and virt-admin shows supported options and commands and as such contains new lines. Both these strings are marked for translation btw. But the way they are formatted now ('\n' being at the start of new line instead at the end of the previous) makes it hard to create a syntax-check rule for 'translation message on one line' (next commit). Reformat both strings a bit (no user visible change though). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 6 ++++-- tools/virt-admin.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d9922a35fc..7b71131db3 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -436,8 +436,10 @@ virshUsage(void) const vshCmdGrp *grp; const vshCmdDef *cmd; - fprintf(stdout, _("\n%1$s [options]... [<command_string>]" - "\n%2$s [options]... <command> [args...]\n\n" + fprintf(stdout, _("\n" + "%1$s [options]... [<command_string>]\n" + "%2$s [options]... <command> [args...]\n" + "\n" " options:\n" " -c | --connect=URI hypervisor connection URI\n" " -d | --debug=NUM debug level [0-4]\n" diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 15a639f1ea..1e22a3c8a9 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1242,8 +1242,10 @@ vshAdmUsage(void) const vshCmdGrp *grp; const vshCmdDef *cmd; - fprintf(stdout, _("\n%1$s [options]... [<command_string>]" - "\n%2$s [options]... <command> [args...]\n\n" + fprintf(stdout, _("\n" + "%1$s [options]... [<command_string>]\n" + "%2$s [options]... <command> [args...]\n" + "\n" " options:\n" " -c | --connect=URI daemon admin connection URI\n" " -d | --debug=NUM debug level [0-4]\n" -- 2.41.0
On Mon, Sep 04, 2023 at 10:31:47 +0200, Michal Privoznik wrote:
The --help output of virsh and virt-admin shows supported options and commands and as such contains new lines. Both these strings are marked for translation btw. But the way they are formatted now ('\n' being at the start of new line instead at the end of the previous) makes it hard to create a syntax-check rule for 'translation message on one line' (next commit).
Reformat both strings a bit (no user visible change though).
It also makes sense because the formatting will make it look the way how it is actually formatted later. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Okay, this is a shortcut. Our coding style says that error messages are exempt from '80 chars long lines' rule. But in the very same paragraph it is said that all error messages need to be marked for translation (as they might be presented to user). Therefore, the syntax-check rule can check if _("...") is formatted on one line. With exception of _("...\n" ...) (e.g. various outputs from helper binaries like leaseshelper, sshhelper, or daemons like lockd, logd). I believe nobody would chose a substring that contains '\n' for git grep-ping the error message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 64c1e2773e..618c0546aa 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic: && { echo 'newline at end of message(s)' 1>&2; \ exit 1; } || : +sc_prohibit_error_message_on_multiple_lines: + @prohibit='[^N]_\(".*[^\\n]"$$' \ + halt='found error message on multiple lines' \ + $(_sc_search_regexp) + # Look for diagnostics that lack a % in the format string, except that we # allow VIR_ERROR to do this, and ignore functions that take a single # string rather than a format argument. @@ -1386,6 +1391,9 @@ exclude_file_name_regexp--sc_prohibit_raw_virclassnew = \ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ +exclude_file_name_regexp--sc_prohibit_error_message_on_multiple_lines = \ + ^(build-aux/syntax-check\.mk|docs/coding-style.rst) + exclude_file_name_regexp--sc_prohibit_nonreentrant = \ ^((po|tests|examples)/|docs/.*(py|js|html\.in|.rst)|run.in$$|tools/wireshark/util/genxdrstub\.pl|tools/virt-login-shell\.c$$) -- 2.41.0
On Mon, Sep 04, 2023 at 10:31:48 +0200, Michal Privoznik wrote:
Okay, this is a shortcut. Our coding style says that error messages are exempt from '80 chars long lines' rule. But in the very same paragraph it is said that all error messages need to be marked for translation (as they might be presented to user).
Therefore, the syntax-check rule can check if _("...") is formatted on one line. With exception of _("...\n" ...) (e.g. various outputs from helper binaries like leaseshelper, sshhelper, or daemons like lockd, logd). I believe nobody would chose a substring that contains '\n' for git grep-ping the error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 64c1e2773e..618c0546aa 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic: && { echo 'newline at end of message(s)' 1>&2; \ exit 1; } || :
+sc_prohibit_error_message_on_multiple_lines: + @prohibit='[^N]_\(".*[^\\n]"$$' \
This doesn't work as you expect it to work, because the [] operator(?) in regex defines a character set to match . Thus you are exempting any message which ends with a backslash _OR_ with an 'n' rather than ending with "\n".
On Mon, Sep 04, 2023 at 10:46:17 +0200, Peter Krempa wrote:
On Mon, Sep 04, 2023 at 10:31:48 +0200, Michal Privoznik wrote:
Okay, this is a shortcut. Our coding style says that error messages are exempt from '80 chars long lines' rule. But in the very same paragraph it is said that all error messages need to be marked for translation (as they might be presented to user).
Therefore, the syntax-check rule can check if _("...") is formatted on one line. With exception of _("...\n" ...) (e.g. various outputs from helper binaries like leaseshelper, sshhelper, or daemons like lockd, logd). I believe nobody would chose a substring that contains '\n' for git grep-ping the error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 64c1e2773e..618c0546aa 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic: && { echo 'newline at end of message(s)' 1>&2; \ exit 1; } || :
+sc_prohibit_error_message_on_multiple_lines: + @prohibit='[^N]_\(".*[^\\n]"$$' \
This doesn't work as you expect it to work, because the [] operator(?) in regex defines a character set to match . Thus you are exempting any message which ends with a backslash _OR_ with an 'n' rather than ending with "\n".
The simplest solution (rather than trying to fiddle with negative lookahead) seems to be: sc_prohibit_error_message_on_multiple_lines: @prohibit='[^N]_\(".*"$$' \ exclude='\\n"$$' \ halt='found error message on multiple lines' \ $(_sc_search_regexp) And perhaps adding a comment explaining what it actually does, so the next guy doesn't have to try to understand it. With the above tweak and comment added: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa