Identify and report checking errors to users #4

rtobar · 2024-11-13T05:34:56+01:00

rtobar commented

2024-11-13 05:34:56 +01:00

powrap --check currently only exists with non-zero status if valid, existing pofiles would wrap. In the case of non-existing, or invalid, pofiles no errors are issued. This prevented us in the Spanish CPython documentation project from identifying an invalid pofile for over a year.

This PR identifies such errors, and allows users to get a non-zero status from powra --check. Since a couple of tests were verifying that indeed no errors were raised, I assumed this was a conscious decision, and therefore the old behaviour is still the default one.

See commit messages for further details, including a related PR made for polib.

powrap --check currently only exists with non-zero status if valid, existing pofiles would wrap. In the case of non-existing, or invalid, pofiles no errors are issued. This prevented us in the Spanish CPython documentation project from identifying an invalid pofile for over a year. This PR identifies such errors, and allows users to get a non-zero status from powra --check. Since a couple of tests were verifying that indeed no errors were raised, I assumed this was a conscious decision, and therefore the old behaviour is still the default one. See commit messages for further details, including a related PR made for polib.

rtobar added 2 commits

2024-11-13 05:34:58 +01:00

Report "would wrap" cases and errors separately b712c872d0

When checking for correct file wrapping, some errors can occur that are
currently silenced. However these errors can be important, and users
might want to learn about them. With the code as it is, they currently
won't always necessarily notice them, since powrap exits with a non-zero
status only if a valid, existing pofile would wrap, but not in other
cases.

This commit adds a new --report-errors command line option. When using
it, these other internal errors (e.g., if a given file does not exist)
influence the exit status of the tool, allowing automatic executions of
powrap to more visibly raise awareness of such issues.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

Report msgcat execution errors as such

ci/woodpecker/pr/woodpecker Pipeline is running

Details

4f2f17a280

In the Spanish translation of the CPython documentation we had a .po
file with a msgstr entry missing its starting double quotes. That
resulted in msgcat failing to execute, however powrap reported no
errors.

This commits considers these situations as errors, allowing powrap
finish with an positive exit status (when using the new --report-errors
command line option).

The rest of the tools we use did *not* report this error though. This
is because polib does not correctly check for msgid/msgstr/etc's content
to be delimited by double quotes, I've opened a PR to fix this [1], but
I'm not sure how successful I'll be with getting it merged, let alone
getting a new version of polib published in PyPI, as the project seems
to be in fairly low maintenance mode.

[1] https://github.com/izimobil/polib/pull/161

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

mdk reviewed

2024-11-13 06:31:06 +01:00

powrap/powrap.py Outdated

					
				@ -177,0 +187,4 @@

				            args.po_files, args.no_wrap, args.quiet, args.diff

				        )

				        failures = would_rewrap

				        if args.report_errors:

Thanks for your PR!

I'm not a fan of hiding errors by default.

What about returning 1 in case of file to be rewrapped and 2 in case of errors?

Thanks for your PR! I'm not a fan of hiding errors by default. What about returning `1` in case of file to be rewrapped and `2` in case of errors?

More than happy! That would've been my normal preference as well, but since that would be breaking behaviour, and because so saw the explicit tests against msgcat failures, I went down the conservative route. I'll rework the changes then. Would you rather I force push (my preference) or build commits on too of what I already pushed?

Also, in that case, would you be happy for me to modify the behaviour of fix_style so it doesn't swallow CalledProcessErrors?

Also, in that case, would you be happy for me to modify the behaviour of `fix_style` so it doesn't swallow `CalledProcessError`s?

I do not care between contributors pushing a new commit or force-pushing an amended commit, I may "rebase-and-squash" at merge time if there's too much small commits though, while if the history is already clean I may just rebase or merge without feeling the need to squash.

I'm OK for fix_style to report its errors too, so we can exit appropriately. I bet the sys.exit from fix_style can be moved outside of it, which would be less surprising in case someone use the function.

Having only main exit would be clean, that's probably what you're intending when writing about "not swallowing CalledProcessError".

I do not care between contributors pushing a new commit or force-pushing an amended commit, I may "rebase-and-squash" at merge time if there's too much small commits though, while if the history is already clean I may just rebase or merge without feeling the need to squash. I'm OK for fix_style to report its errors too, so we can exit appropriately. I bet the sys.exit from fix_style can be moved outside of it, which would be less surprising in case someone use the function. Having only `main` exit would be clean, that's probably what you're intending when writing about "not swallowing CalledProcessError".

Thanks @mdk for the feedback. I've reworked the changes, removing the --report-errors flag and exiting with ecode 2 when these occur. I've also removed the call to sys.exit(127) from within the fix/check_style functions and moved it upwards to the main function.

Thanks @mdk for the feedback. I've reworked the changes, removing the `--report-errors` flag and exiting with ecode 2 when these occur. I've also removed the call to `sys.exit(127)` from within the fix/check_style functions and moved it upwards to the main function.