Skip to content

Conversation

@cfournie
Copy link
Contributor

@cfournie cfournie commented Mar 7, 2017

This PR tries to measure the complexity of try/except/finally blocks in terms of the number of AST nodes within the body of each statement. The goal of these rules is to encourage less code (not simply measured in lines) within these sections.

The larger the body of the try, the more likely that an exception will be raised by a line of code that you didn't expect to raise an exception. In those cases, the try/except block hides a real error.

– Google Python Styleguide

Running this on a large codebase we get no parse errors and by setting the max length to zero we also get some statistics on body tree sizes in nodes:

Try Except Finally
Count 122 126 12
90th precentile 40.8 33 15.7
80th precentile 24.8 23 12.8
70th precentile 18 17 12
Min 1 2 4
Max 104 111 29
Mean 17 16 11
Median 10 11 10

Assuming that the 80% of this large project's try/except/finally blocks are appropriate, the default block sizes could be set to 25, 23, and 13 respectively.

@cfournie cfournie changed the title [WIP] Add try/except/finally complexity rules Add try/except/finally complexity rules Mar 7, 2017
@cfournie cfournie changed the title Add try/except/finally complexity rules [WIP] Add try/except/finally complexity rules Mar 7, 2017
@cfournie cfournie changed the title [WIP] Add try/except/finally complexity rules Add try/except/finally complexity rules Mar 8, 2017
with self.assertAddsMessages(
pylint.testutils.Message('finally-too-long', node=root.body[0], args={'found': 24}),
pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 28}),
pylint.testutils.Message('except-too-long', node=root.body[0].body[0].handlers[0],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you name these nodes something more meaningful, so the reader can know what root.body[0].body[0].handlers[0] refers to without parsing the tree?

pylint.testutils.Message('finally-too-long', node=root.body[0], args={'found': 24}),
pylint.testutils.Message('try-too-long', node=root.body[0].body[0], args={'found': 28}),
pylint.testutils.Message('except-too-long', node=root.body[0].body[0].handlers[0],
args={'found': 39}),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard-coding these specific values make this test a little fragile, leaning on the specific implementation of count_tree_size. Is there any way to test that the messages were thrown with a value, not necessarily this specific value?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tied specifically to the value of the test input, which is unlikely to change without the test input also changing. I think it's fine to be really specific here.

@JasonMWhite
Copy link

Minor comments about one of the tests, otherwise LGTM

@honkfestival
Copy link

LGTM too pending @JasonMWhite's comments

@cfournie cfournie force-pushed the exception_complexity branch from 7d9fc55 to a25a373 Compare March 8, 2017 22:15
@cfournie cfournie merged commit 6195178 into master Mar 8, 2017
@cfournie cfournie deleted the exception_complexity branch March 8, 2017 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants