[ADD] except-pass: Emit message If a except:pass is used (OCA/#252) #107

JesusZapata · 2017-01-18T19:05:06Z

The pass into block except is not a good practice!

By including the pass we assume that our algorithm can continue to function after the exception occurred

If you really need to use the pass consider logging that exception

    try:
        sentences
    except:
        pass
       _logger.debug('Why the exception is safe....', exc_info=1))

fix OCA/maintainer-tools#252

JesusZapata · 2017-01-19T20:18:41Z

@moylop260
What do you think about this PR?

moylop260 · 2017-01-19T20:39:06Z

pylint_odoo/checkers/modules_odoo.py

+    def visit_tryexcept(self, node):
+        """Visit block try except"""
+        for handler in node.handlers:
+            if handler.body:


use and instead of nested if (in order to reduce mccabe rate)

@moylop260
Thanks!

moylop260 · 2017-01-24T23:29:08Z

pylint_odoo/checkers/modules_odoo.py

+    def visit_tryexcept(self, node):
+        """Visit block try except"""
+        for handler in node.handlers:
+            if handler.body and isinstance(handler.body[0],


And len(handler.body) == 1 because we need validate just that unique sentence.

try: code except: pass logger # this case is valid, is other error but is valid for this one

@moylop260
Done!

moylop260 · 2017-01-25T22:23:10Z

pylint_odoo/checkers/modules_odoo.py

+    def visit_tryexcept(self, node):
+        """Visit block try except"""
+        for handler in node.handlers:
+            if len(handler.body) == 1 and \


Avoid use \ better use (

if (len(handler.body) == 1 and isinstace(...))

@moylop260
Done!
Thanks!

moylop260 · 2017-01-26T17:13:20Z

Could you fix conflicts?

JesusZapata · 2017-01-26T18:05:34Z

@moylop260
Done! I fixed this conflict!

moylop260 · 2017-01-26T18:23:13Z

@lasley @pedrobaeza @dreispt @lmignon
your feedback is welcome

lasley · 2017-01-26T18:32:25Z

Hrmmmm I'm torn on this one. Forgive my ignorance in pylint, but is it possible to check if the exception is used slightly later in code? I use a strategy similar to below fairly often:

err = None
for i in iter_of_callables:
    try:
        i()
    except IException as err:
        pass
if err:
    raise err

Context for this weirdness is in some of my connector modules, where you want to show the error after the fact but still forcefully continue with the queued ops.

moylop260 · 2017-01-27T00:16:50Z

That is a good point.
@JesusZapata
Could you support that case, please?

lasley · 2017-01-27T00:20:41Z

Yeah maybe we just leverage the pre-existing check to see whether a variable is used in the namespace? If the exception is assigned to a var, and the var is used, we can assume the exception is handled properly - or following the Zen of an explicit silence.

Thanks for taking this into consideration!

dreispt · 2017-01-27T13:47:31Z

Than could be just a lint warning message, not breaking the build.

moylop260 · 2017-01-27T15:49:26Z

pylint_odoo/checkers/modules_odoo.py

+    @utils.check_messages('except-pass')
+    def visit_tryexcept(self, node):
+        """Visit block try except"""
+        for handler in node.handlers:


Here you could validate if the except has a as in order to skip the check because the new variable could be used.
It this variable is not used then we will have a variable-unused from other check

This is the easiest way to skip the check using the @lasley recommendation

@moylop260 @lasley
Done, Now we exclude if the exception is assigned to one variable

moylop260 · 2017-01-27T18:37:13Z

@dreispt Than could be just a lint warning message, not breaking the build.

Yeah, from MQT we could enabled it like as a beta check

moylop260 · 2017-01-31T01:45:30Z

pylint_odoo/checkers/modules_odoo.py

+        for handler in node.handlers:
+            if (len(handler.body) == 1 and
+                    isinstance(handler.body[0], astroid.node_classes.Pass) and
+                    not isinstance(node.handlers[0].name,


and node.handlers and in order to avoid IndexError using node.handlers[0]

@moylop260
Done, added another condition to if.

moylop260 · 2017-01-31T01:48:50Z

pylint_odoo/test_repo/test_module/except_pass.py

+        """Test method """
+        try:
+            raise Exception('Exception')
+        except Exception:


Add other test case when there aren't handlers.

try: raise Exception('Exception') except Exception:

and other ones using many handlers

try: raise Exception('Exception') except Exception, UserError:

try: raise Exception('Exception') except (Exception, UserError) as exception:

@moylop260
Done,
This cases work fine.

moylop260 · 2017-01-31T02:18:56Z

pylint_odoo/checkers/modules_odoo.py

+        'except-pass',
+        settings.DESC_DFLT
+    ),
+    'W%d38' % settings.BASE_OMODULE_ID: (


The except-pass should be 38 not 37

JesusZapata · 2017-01-31T03:36:15Z

@moylop260
I have attached more cases and now check if there are several handlers.

moylop260 · 2017-01-31T15:39:17Z

pylint_odoo/test_repo/test_module/except_pass.py

+            raise Exception('Exception')
+        except (Exception, IndexError, NameError), exception:
+            pass
+        except Exception:


Add a comment where the message should be emitted:
except Exception: # except-pass

moylop260 · 2017-01-31T15:39:46Z

pylint_odoo/test_repo/test_module/except_pass.py

+    def test_4_method(self):
+        try:
+            raise Exception('Exception')
+        except Exception, UserError:


Use snake_name for UserError

moylop260 · 2017-01-31T15:40:20Z

pylint_odoo/test_repo/test_module/except_pass.py

+    def test_5_method(self):
+        try:
+            raise Exception('Exception')
+        except (Exception, IndexError) as exception:


Add the case where there are many Exceptions but without alias except (Exception, IndexError):

moylop260 · 2017-01-31T15:40:49Z

pylint_odoo/checkers/modules_odoo.py

+        """Visit block try except"""
+        for handler in node.handlers:
+            if (not handler.name and
+                    len(handler.body) and


s/len(handler.body)/handler.body/g

JesusZapata · 2017-01-31T15:56:13Z

@moylop260
I have done all changes
Thanks for you comments

moylop260 · 2017-01-31T16:14:17Z

pylint_odoo/test_repo/test_module/except_pass.py

+        try:
+            raise Exception('Exception')
+        except Exception:
+            print('Exception')


Use pass before of print (this case shouldn't emit error)

@moylop260
Done,
I have validated the case of multiple handler.body

moylop260 · 2017-01-31T20:16:51Z

@lasley
Could you review the case requested, please?

moylop260 · 2017-01-31T20:17:24Z

@JesusZapata
Could you squash all commits in one, please?

[FIX] pylint-odoo: Best use of if clause [FIX] pylint-odoo: Validate the body contain only one line [FIX] pylint-odoo: Avoid use \ better use ( [REF] incoherent-interpreter-exec-perm: Better message (Vauxoo#106) [ADD] xml-attribute-translatable: Check XML attribute without translation parameter (Vauxoo#105) Close OCA#104 [REF] javascript-lint: Use eslint instead of jshint (Vauxoo#97) [ADD] renamed-field-parameter: Detect deprecated field values (digits_compute, select) (Vauxoo#99) [FIX] Pep8 check and bad index for ODOO_MSGS [ADD] attribute-string-redundant: Check if "string" parameter is equal to variable name (Vauxoo#100) [FIX] attribute-string-redundant: Add "isinstance" validation for nodes Fix OCA#109 [IMP] Exclude exception when use as assignation [FIX] Pep8 check local variable 'exception' [FIX] Pep8 blank line at end of file [FIX] Adding more cases of test [REF] Supporting more cases of exception [FIX] Modify file main.py for adding new case of except-pass [IMP] Adding another tests case [FIX] Adding another test case and better validation [FIX] Delete unnecessary condition of if [FIX] Fix comment line

JesusZapata · 2017-01-31T21:00:48Z

@moylop260
I made rebase.

moylop260 · 2017-02-01T18:56:53Z

@lasley Do you have any comment about?

lasley

Excellent, this covers my reservations fully - thank you.

I'm kind of unclear on the test naming/comments though. Could we add some sort of indicator as to which is pass/fail, and why? SOmething like:

def test_valid_1(self):
    """ It should not fail if the exception is assigned & used. """

JesusZapata · 2017-02-01T19:39:06Z

@lasley
Only documented when skip the except-pass

JesusZapata mentioned this pull request Jan 18, 2017

[IMP] Adding exception section for good practice OCA/maintainer-tools#252

Merged

moylop260 reviewed Jan 19, 2017

View reviewed changes

moylop260 approved these changes Jan 19, 2017

View reviewed changes

moylop260 reviewed Jan 24, 2017

View reviewed changes

moylop260 reviewed Jan 25, 2017

View reviewed changes

moylop260 approved these changes Jan 26, 2017

View reviewed changes

moylop260 added need review and removed need review labels Jan 26, 2017

moylop260 reviewed Jan 27, 2017

View reviewed changes

moylop260 assigned JesusZapata Jan 30, 2017

moylop260 reviewed Jan 31, 2017

View reviewed changes

moylop260 requested changes Jan 31, 2017

View reviewed changes

moylop260 reviewed Jan 31, 2017

View reviewed changes

JesusZapata changed the title ~~[IMP] except-pass: Emit message If a except:pass is used (OCA/#252)~~ [ADD] except-pass: Emit message If a except:pass is used (OCA/#252) Jan 31, 2017

moylop260 approved these changes Jan 31, 2017

View reviewed changes

JesusZapata force-pushed the oca-master-add-except-pass-checker-jesuszapata branch from 6a6eedc to 24fd5fb Compare January 31, 2017 20:51

JesusZapata force-pushed the oca-master-add-except-pass-checker-jesuszapata branch from 5531bd3 to daf67df Compare January 31, 2017 20:56

This was referenced Jan 31, 2017

[ADD] except-pass: Emit message If a "except:pass" is used Vauxoo/pylint-odoo#106

Closed

[ADD] except-pass: New check to detect sentence "pass" in exceptions catched Vauxoo/pylint-odoo#101

Closed

lasley approved these changes Feb 1, 2017

View reviewed changes

[ADD] More explicit comment when skip the check except-pass

279763f

moylop260 merged commit bc1e6f4 into OCA:master Feb 7, 2017

moylop260 deleted the oca-master-add-except-pass-checker-jesuszapata branch February 7, 2017 15:59

StefanRijnhart mentioned this pull request Feb 1, 2022

Opinion: except-pass conflicts with Pythonic EAFP #361

Closed

antonag32 mentioned this pull request Oct 2, 2023

[REF] Filter out "bump version" from CHANGELOG #472

Merged

[ADD] except-pass: Emit message If a except:pass is used (OCA/#252) #107

[ADD] except-pass: Emit message If a except:pass is used (OCA/#252) #107

Conversation

JesusZapata commented Jan 18, 2017 • edited Loading

JesusZapata commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moylop260 commented Jan 26, 2017

JesusZapata commented Jan 26, 2017

moylop260 commented Jan 26, 2017

lasley commented Jan 26, 2017 • edited Loading

moylop260 commented Jan 27, 2017

lasley commented Jan 27, 2017

dreispt commented Jan 27, 2017

moylop260 Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moylop260 commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesusZapata commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesusZapata commented Jan 31, 2017

moylop260 Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moylop260 commented Jan 31, 2017

moylop260 commented Jan 31, 2017

JesusZapata commented Jan 31, 2017

moylop260 commented Feb 1, 2017

lasley left a comment • edited Loading

Choose a reason for hiding this comment

JesusZapata commented Feb 1, 2017

JesusZapata commented Jan 18, 2017 •

edited

Loading

lasley commented Jan 26, 2017 •

edited

Loading

moylop260 Jan 27, 2017 •

edited

Loading

moylop260 Jan 31, 2017 •

edited

Loading

lasley left a comment •

edited

Loading