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.
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>
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>
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?
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?
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.
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.
@ -177,0 +187,4 @@args.po_files, args.no_wrap, args.quiet, args.diff)failures = would_rewrapif args.report_errors:Thanks for your PR!
I'm not a fan of hiding errors by default.
What about returning
1in case of file to be rewrapped and2in 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_styleso it doesn't swallowCalledProcessErrors?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
mainexit 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-errorsflag and exiting with ecode 2 when these occur. I've also removed the call tosys.exit(127)from within the fix/check_style functions and moved it upwards to the main function.4f2f17a2801580de90641580de9064751f352e6eThanks for the PR @rtobar! I'll try to publish a release.
Release published!
Thanks @mdk, this is much appreciated!