[IMP] add new lint: no-raise-unlink #458
Conversation
|
Is it related to issue? |
Yes, but it needs to be moved from oca-pre-commit-hooks: |
antonag32
force-pushed
the
anton-no-raise-unlink
branch
2 times, most recently
from
77934bc
to
588c0f7
Compare
antonag32
changed the title
|
I looked into taking into account functions calls inside unlink that down the line can generate raises, but after some testing I deemed it not practical to do it so I left it as this, it checks for direct |
|
Please, review |
| def get_enclosing_function(node, depth=10): | ||
| count = 0 | ||
| parent = getattr(node, "parent", False) | ||
| while not isinstance(parent, FunctionDef) and count < depth: |
There was a problem hiding this comment.
I'd suggest using while's when there's no other alternative. In this case, this could be achieved by using
range(depth)
On the other hand, why is it required to recursively check parents?
There was a problem hiding this comment.
Parents are recursively checked to find the enclosing function, for example:
def unlink(self):
if self.business_logic():
raise ValueError() # offending node, will be reviewed in visit_raiseIn that case the initial node will be the raise but we need to know if it is inside an unlink function so the solution I came up with was to go up the parent tree (effectively up in terms of code lines) until a function definition is found (or the lookup limit is reached).
|
|
||
| def unlink(self): | ||
| if self.name == 'explode': | ||
| raise RuntimeError |
There was a problem hiding this comment.
| raise RuntimeError | |
| raise RuntimeError() |
Same in other exceptions
| extra_params = ["--disable=all", "--enable=no-raise-unlink"] | ||
| self.assertDictEqual( | ||
| self.run_pylint([os.path.join(self.root_path_modules, "eleven_module")], extra_params).linter.stats.by_msg, | ||
| {"no-raise-unlink": 2}, |
There was a problem hiding this comment.
Is this for v11?
Take into account this lint only applies for version 15.0 and above.
There was a problem hiding this comment.
I moved it out to test_module which still has a 10.0 manifest but it seems like that is not very well respected, still may seem less confusing since it has no version in its name.
Also added more code to the test case to ensure this check only runs on 15.0 and above.
|
FYI @antonag32, you can close issues from other repositories by using:
|
antonag32
force-pushed
the
anton-no-raise-unlink
branch
3 times, most recently
from
22f7370
to
d24f8b3
Compare
|
Thanks for the review, I made the requested changes, can you check @luisg123v |
| class FunctionChecker(OdooBaseChecker): | ||
| name = "odoolint" | ||
| msgs = ODOO_MSGS | ||
| checks_maxmin_odoo_version = {"no-raise-unlink": {"odoo_minversion": "15.0"}} |
There was a problem hiding this comment.
Could you split this line, please? So it will be easier to add future lints or more conditions.
| checks_maxmin_odoo_version = {"no-raise-unlink": {"odoo_minversion": "15.0"}} | |
| checks_maxmin_odoo_version = { | |
| "no-raise-unlink": { | |
| "odoo_minversion": "15.0", | |
| }, | |
| } |
Besides, it's more readable.
| parent = getattr(node, "parent", False) | ||
| for _i in range(depth): | ||
| if not parent or isinstance(parent, FunctionDef): | ||
| return parent |
There was a problem hiding this comment.
I think we could just break and return parent on any case after this loop, IMHO it's clearer that way.
|
The following is more of a suggestion, but: I think closes statements fit better at the bottom, because they works like directives, while the commit description is the actual meesage one expects to read. |
antonag32
force-pushed
the
anton-no-raise-unlink
branch
2 times, most recently
from
6140642
to
df499f4
Compare
Raising errors inside unlink(), especially when inherited can leave stale data behind, therefore they are prohibited. This lint checks for that. Closes OCA/odoo-pre-commit-hooks#73. Context: odoo/odoo@1c8a958
antonag32
force-pushed
the
anton-no-raise-unlink
branch
from
df499f4
to
b8a40dc
Compare
|
I applied the requested changes. Also thought about it and adding an extra file did not seem to make much sense anymore. I added that file thinking the checker would store some state (functions that raise exceptions) and that it was clearer that way. However the checker no longer stores any state and it is actually simple, so I don't think it grants a new file. Also made some docstring fixes/refactoring around the original code where this new check was added. |
Closes OCA/odoo-pre-commit-hooks#73.
Context: odoo/odoo@1c8a958
Raising errors inside unlink(), especially when inherited can leave
stale data behind, therefore they are prohibited. This lint checks
for that.