Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some unnecessary empty return statements #455

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

rahulporuri
Copy link
Contributor

This PR removes a number of unnecessary empty return statements from the codebase - from enable, from kiva, from the test suite and from the examples.

Apologies to the reviewer because this is quite an ugly PR to review. I checked each return statement to ensure that the empty statement is not part of conditional logic/early return from a function/method. Only empty returns at the end of functions/methods are removed.

Note that this commit does not remove all of the unnecessary empty
return statements
Copy link
Member

@jwiggins jwiggins left a comment

Choose a reason for hiding this comment

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

It seems like there should be a tool that can do this for us...

	modified:   enable/viewable.py
Comment on lines 17 to 19
# This overrides the default Component request_redraw by asking
# all of the views to redraw themselves.
return
Copy link
Member

Choose a reason for hiding this comment

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

You can also just change the comment to a docstring so that the return can still be removed.

@codecov-io
Copy link

codecov-io commented Dec 1, 2020

Codecov Report

Merging #455 (7cdfcb0) into master (118a04a) will decrease coverage by 1.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   30.26%   29.14%   -1.12%     
==========================================
  Files         206      206              
  Lines       18143    17954     -189     
  Branches     2461     2466       +5     
==========================================
- Hits         5491     5233     -258     
- Misses      12309    12394      +85     
+ Partials      343      327      -16     
Impacted Files Coverage Δ
enable/abstract_overlay.py 39.39% <ø> (+3.28%) ⬆️
enable/abstract_window.py 53.43% <ø> (-0.84%) ⬇️
enable/base.py 45.94% <ø> (+1.04%) ⬆️
enable/base_tool.py 76.74% <ø> (+0.14%) ⬆️
enable/canvas.py 27.65% <ø> (+1.06%) ⬆️
enable/compass.py 30.48% <ø> (+1.21%) ⬆️
enable/component.py 54.09% <ø> (-1.58%) ⬇️
enable/component_editor.py 78.72% <ø> (-4.61%) ⬇️
enable/container.py 55.33% <ø> (+0.31%) ⬆️
enable/coordinate_box.py 31.65% <ø> (-0.54%) ⬇️
... and 136 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 118a04a...563b62e. Read the comment docs.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM. I reviewed the code surrounding the removal and confirmed (the best I could) that they are all at the end of a function (not early termination, and not function whose API explicitly requires a None to be returned)

@@ -1028,7 +1006,6 @@ def _set_active_tool(self, tool):
tool._activate()

self.invalidate_and_redraw()
return
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one more return at the bottom of _old_dispatch which can be removed.

@@ -55,7 +53,6 @@ def _mouse_move_changed(self, event):
if (x != self._cur_x) or (y != self._cur_y):
self._cur_x, self._cur_y = x, y
self.redraw()
return
Copy link
Contributor

Choose a reason for hiding this comment

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

The return at the bottom of _draw (next function) can be removed too.

@@ -249,7 +249,6 @@ def __touch__(self, key):
self.__order__ = self.__order__[1:] + [key]
else:
self.__order__.append(key)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

No action. Just an observation that this will conflict with #392
(There are probably plenty of opportunity for merge conflicts anyway.)

@kitchoi
Copy link
Contributor

kitchoi commented Dec 7, 2020

Since this PR was opened, a number of PRs have gone in, including one that updates the CI configuration to require tests to pass on wx. As a sanity check, it would be good to merge master in here to get another CI run before we merge.

@kitchoi kitchoi merged commit 3d323c8 into master Dec 8, 2020
@kitchoi kitchoi deleted the cln/remove-some-empty-returns branch December 8, 2020 14:13
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.

4 participants