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.

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

Conversation

Kroisse
Copy link

@Kroisse Kroisse commented May 16, 2019

  • Added maximum required versions to all test dependencies.
  • Removed redundant flake8 execution. It will be run by pytest-flake8 plugin.
  • Exit quitely on KeyboardInterrupt.

Blocks #11 and #14.

@Kroisse Kroisse requested review from kanghyojun and jangjunha May 16, 2019 06:36
@Kroisse Kroisse self-assigned this May 16, 2019
@@ -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.

@dahlia
Copy link
Contributor

dahlia commented May 16, 2019

Although I approved, I don't have permission to merge this.

No more traceback while pressing Ctrl+C.
@Kroisse
Copy link
Author

Kroisse commented May 16, 2019

https://travis-ci.org/spoqa/geofront-cli/jobs/533189683

$ tox
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.3.6/bin/tox", line 7, in <module>
    from tox import cmdline
  File "/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages/tox/__init__.py", line 32, in <module>
    from .session import cmdline  # isort:skip
  File "/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages/tox/session/__init__.py", line 20, in <module>
    from tox.config import parseconfig
  File "/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages/tox/config/__init__.py", line 18, in <module>
    import pkg_resources
  File "/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages/pkg_resources/__init__.py", line 92, in <module>
    raise RuntimeError("Python 3.4 or later is required")
RuntimeError: Python 3.4 or later is required

……Time to consider dropping support for 3.3?

@dahlia
Copy link
Contributor

dahlia commented May 16, 2019

Python 3.3 support now seems unrealistic. How about dropping this and bumping the minor version?

@Kroisse
Copy link
Author

Kroisse commented May 16, 2019

It makes sense.

Kroisse added 5 commits May 16, 2019 18:05
xcode8.1 already obsoleted. Previous builds are fallen back to default version (xcode9.4)
@Kroisse Kroisse force-pushed the fix-lint branch 2 times, most recently from 3ad7392 to 8c4f0b3 Compare May 16, 2019 14:57
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