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
Add Python 3.7 support #39
Conversation
Hi @mattbennett , please take a look when you have a moment. I've added Python 3.7 tests to start preparing to support it since that's what we would like to do at Sohonet soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One comment/question
test/conftest.py
Outdated
@@ -40,7 +40,7 @@ def make_container(service_cls, config): | |||
for c in all_containers: | |||
try: | |||
c.stop() | |||
except: | |||
except: # noqa: E722 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want except: Exception
here. Can't see a reason why we should be catching anything else.
Also, I wonder why we are reimplementing the container_factory
here. Does nameko-sqlalchemy
require stop
instead of kill
for teardown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattbennett You're right about the exception and we do not seem to require stop
instead of kill
either. I have removed the fixture and all tests are passing without reimplementing it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I poked around in the history and it turns out that this was ported from Nameko core before Nameko had its own pytest plugin, so that explains the need for reimplementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Good to remove old code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!
Thanks @mattbennett ! Please merge it if you're happy. |
Preparing the library for Python 3.7.
After updating the
dev
requirements, it seems to work fine and all the tests pass successfully.xenial
sudo
Travis keyword has been fully deprecateddev
requirements to the latest version:pylint
for Python 2pep8
after upgradingflake8
pytest
dev
extras, remove unused code.mysql
Travis service since we're user Docker containerspytest
references in the documentation