[FIX] Remove unnecessary double log #31699
Conversation
yajo
force-pushed
the
11.0-remove-unnecessary-logger
branch
from
0a413b3
to
5e4e746
Compare
|
I forgot to mention that this would help downstream because in @OCA we check that there are no It's OK to fix the v10 branches, but for v11 it's a lot of work that's actually not going to add value because Python 3 does the dirty job by himself here. Example PR that fixes our Travis: OCA/stock-logistics-warehouse#552 🙏 Please merge this for v11+. Thanks! |
|
@odony please check this as it's breaking all OCA constraints tests. In v10, we will need to do this any way, but the benefit is more than the harm, but for v11/v12, being unneeded, it's a pain. |
Reverts 2b1d3ff introduced in 10.0 via It was relatively useful in Odoo 10.0 because in Python 2 the exception was missing a root cause traceback. But Python 3 includes exception chaining by default, so it comes for free. See [PEP3134](https://legacy.python.org/dev/peps/pep-3134/) On top of being redundant in P3, it can also break some testcases by causing an extra ERROR log entry, even when the final exception is expected and caught. So it's simpler to remove it.
odony
force-pushed
the
11.0-remove-unnecessary-logger
branch
from
5e4e746
to
96cd92f
Compare
There was a problem hiding this comment.
So double work, hmm? I think I made a mistake when I merged the patch in 10.0 in the first place - the risk/benefit ratio wasn't good. Even with very low risk (only impacts tests), the benefit was quite low too, so it was kind of a waste of energy (now twice). Let this serve as a reminder of merge policies and why they exist ;)
As for targeting 11.0, it's not pure P3 yet, but I guess it's acceptable that 11.0 on P2 gives slightly less detailed logs. I have slightly amended the commit to fix the format, so we can have robodoo merge it as is.
@robodoo r+
|
Thanks for the quick reaction. There's no better option for everything... |
Reverts 2b1d3ff introduced in 10.0 via It was relatively useful in Odoo 10.0 because in Python 2 the exception was missing a root cause traceback. But Python 3 includes exception chaining by default, so it comes for free. See [PEP3134](https://legacy.python.org/dev/peps/pep-3134/) On top of being redundant in P3, it can also break some testcases by causing an extra ERROR log entry, even when the final exception is expected and caught. So it's simpler to remove it. closes #31699 Signed-off-by: Olivier Dony (odo) <odo@openerp.com>
robodoo
added
CI 🤖
|
Merged, thanks! |
Before this patch, in #28612 a new feature was added that helped the admin to know what validation had actually failed when it happened.
It was needed in Odoo 10.0 because in Python 2, the exception didn't include the original traceback, but in Odoo 11+, with Python 3, PEP3134 saves us this need.
This new logger makes some downstream tests to fail if the testbots expect no
ERRORmessages; and in any case the logs can be confusing by logging anERRORwhere there might be aexceptabove in the call stack that expects and reacts to such failure.To avoid all of this, let's revert 2b1d3ff in Python 3 versions of Odoo.
--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
@Tecnativa @etobella