Skip to content

Fix broken CI test, apply new lint rules #15

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.

Sign up for GitHub

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
+44 −53
Open

Fix broken CI test, apply new lint rules #15

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d672877
Pin test dependencies
Kroisse May 16, 2019
a50df2b
Fix lint errors
Kroisse May 16, 2019
2122772
Don't need to run flake8 twice
Kroisse May 16, 2019
2f1c3f8
Handle KeyboardInterrupt as normal exit
Kroisse May 16, 2019
608d400
Drop support for Python 3.3, PyPy 5.3 or below
Kroisse May 16, 2019
3bd2c99
Add support for Python up to 3.7 and PyPy 7.1
Kroisse May 16, 2019
c198aae
Jump to Xenial
Kroisse May 16, 2019
59abb85
Shorten the build matrix
Kroisse May 16, 2019
7eca69d
Retain available Python versions
Kroisse May 16, 2019
2dc001a
Upgrade to xcode9.4
Kroisse May 16, 2019
80a112f
Use xcode10.2
Kroisse May 16, 2019
ef4f1c8
Fix macOS test to install zlib explicitly
Kroisse May 16, 2019
00127a6
Just use builtin pyenv and tox
Kroisse May 16, 2019
0d9b0f0
Install sqlite3 and make pyenv verbose
Kroisse May 16, 2019
37e7ff4
Locate the path to sqlite3
Kroisse May 16, 2019
cefa862
Try to trust Travis
Kroisse May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions geofrontcli/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

from .key import PublicKey
from .ssl import create_urllib_https_handler
from .version import MIN_PROTOCOL_VERSION, MAX_PROTOCOL_VERSION, VERSION
from .version import MAX_PROTOCOL_VERSION, MIN_PROTOCOL_VERSION, VERSION

__all__ = ('REMOTE_PATTERN', 'BufferedResponse',
'Client', 'ExpiredTokenIdError',
Expand Down Expand Up @@ -197,7 +197,7 @@ def remotes(self):
extra={'user_waiting': False})
return dict((alias, fmt(remote))
for alias, remote in result.items())
except:
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, it was intended to catch KeyboardInterrupt as well. It's better catch BaseException here IMO.

Copy link
Author

Choose a reason for hiding this comment

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

Catching BaseException is still harmful because BaseException includes non-error exceptions like StopIteration or GeneratorExit. I think it is better to catch Exception and KeyboardInterrupt explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not the case here, because the only thing this except: clause does is just logging and reraising. What's your opinion?

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion, it's much better to handle KeyboardInterrupt separately instead treating it as an error, since the situation when KeyboardInterrupt has been raised represents the obvious intention to abort this execution, not an unexpected failure.

logger.info('Failed to fetch the list of remotes.',
extra={'user_waiting': False})
raise
Expand Down Expand Up @@ -227,7 +227,7 @@ def authorize(self, alias):
logger.info('Authentication is required.',
extra={'user_waiting': False})
raise
except:
except Exception:
logger.info('Authorization to %s has failed.', alias,
extra={'user_waiting': False})
raise
Expand Down
11 changes: 5 additions & 6 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ minversion = 1.6.0

[testenv]
deps =
flake8 >= 3.3.0
flake8-import-order-spoqa
pytest >= 3.0.7, < 3.1.0
pytest-flake8 >= 0.8.1, < 0.9.0
testtools
flake8 >= 3.5.0, < 3.8.0
flake8-import-order-spoqa >= 1.5.0, < 2.0.0
pytest >= 3.5.0, < 4.0.0
pytest-flake8 >= 1.0.4, < 1.1.0
testtools >= 2.3.0, < 3.0.0
# testtools is required by dirspec
commands =
flake8 geofrontcli
py.test {posargs:--durations=5}

[pytest]
Expand Down