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
Open

Fix broken CI test, apply new lint rules #15

Show file tree
Hide file tree
Changes from 5 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
7 changes: 2 additions & 5 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,10 @@ matrix:
# See also: https://github.com/Homebrew/homebrew-core/issues/6949
- os: linux
language: python
python: pypy-5.3.1
python: pypy-5.4
- os: linux
language: python
python: 2.7
- os: linux
language: python
python: 3.3
- os: linux
language: python
python: 3.4
Expand All @@ -28,7 +25,7 @@ install:
pip install --upgrade pip setuptools tox-travis;
elif [[ "$TRAVIS_OS_NAME" = "osx" ]]; then
brew tap drolando/homebrew-deadsnakes;
brew install python33 python34 python35 python3 pypy
brew install python34 python35 python3 pypy
pip install --upgrade pip setuptools
pip install --user tox;
fi
Expand Down
7 changes: 7 additions & 0 deletions geofrontcli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,13 @@ def prepend_lines(c, text):


def main(args=None):
try:
_run(args)
except KeyboardInterrupt:
parser.exit()


def _run(args=None):
args = parser.parse_args(args)
log_handler = logging.StreamHandler(sys.stdout)
log_handler.addFilter(UserWaitingFilter())
Expand Down
14 changes: 11 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,11 @@ def remotes(self):
extra={'user_waiting': False})
return dict((alias, fmt(remote))
for alias, remote in result.items())
except:
except KeyboardInterrupt:
logger.info('Request is aborted.',
extra={'user_waiting': False})
raise
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 +231,11 @@ def authorize(self, alias):
logger.info('Authentication is required.',
extra={'user_waiting': False})
raise
except:
except KeyboardInterrupt:
logger.info('Authorization is aborted.',
extra={'user_waiting': False})
raise
except Exception:
logger.info('Authorization to %s has failed.', alias,
extra={'user_waiting': False})
raise
Expand Down
15 changes: 3 additions & 12 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,10 @@ def readme():
'six',
}

below_py34_requires = {
'enum34',
}

win32_requires = {
'pypiwin32',
}

if sys.version_info < (3, 4):
install_requires.update(below_py34_requires)

if sys.platform == 'win32':
install_requires.update(win32_requires)

Expand All @@ -62,7 +55,6 @@ def readme():
''',
install_requires=list(install_requires),
extras_require={
":python_version<'3.4'": list(below_py34_requires),
":sys_platform=='win32'": list(win32_requires),
},
classifiers=[
Expand All @@ -75,7 +67,6 @@ def readme():
'Programming Language :: Python :: 2',
'Programming Language :: Python :: 2.7',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.3',
'Programming Language :: Python :: 3.4',
'Programming Language :: Python :: 3.5',
'Programming Language :: Python :: 3.6',
Expand All @@ -90,7 +81,7 @@ def readme():
if 'bdist_wheel' in sys.argv and (
below_py34_requires.issubset(install_requires) or
win32_requires.issubset(install_requires)):
warnings.warn('Building wheels on Windows or using below Python 3.4 is '
'not recommended since platform-specific dependencies can '
'be merged into common dependencies:\n' +
warnings.warn('Building wheels on Windows is not recommended since '
'platform-specific dependencies can be merged into common '
'dependencies:\n' +
'\n'.join('- ' + i for i in install_requires))
13 changes: 6 additions & 7 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
[tox]
envlist = pypy, py27, py33, py34, py35, py36
envlist = pypy, py27, py34, py35, py36
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