-
Notifications
You must be signed in to change notification settings - Fork 188
Drop Python 3.5 support #83
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
Conversation
For removing Python 3.5 you also need to update the setup.py python_requires AFAIK. |
f997cb8
to
7d02557
Compare
@hashhar disregard Mac OS and Windows support... I need to think about whether we want to run docker on each of those platforms or if we should have another service up and running for integration testing. |
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'm not so sure about the f-string changes (others may disagree) - they are readable in some cases but not in others. Also, can IDEs/static analysis tools catch cases where a f-string references a non-existent/mis-spelled variable?
e.g.
return "MAP({}, {})".format(
self._format_prepared_param(keys),
self._format_prepared_param(values)
)
vs
return f"MAP({self._format_prepared_param(keys)},
{self._format_prepared_param(values)})"
```.
Also, are we using mypy in CI to actually verify the type annotations we have?
What end-user behaviour is for people who try to install the package for an unsupported version of Python?
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.
@joshthoward What does a user trying to install on Python3.5 see? Do get the older version? Or does it silently fail?
@hashhar It complains if you try to use Python 3.5. See below: % pyenv virtualenv 3.5.10 test
Requirement already satisfied: setuptools in /Users/joshthoward/.pyenv/versions/3.5.10/envs/test/lib/python3.5/site-packages
Requirement already satisfied: pip in /Users/joshthoward/.pyenv/versions/3.5.10/envs/test/lib/python3.5/site-packages
% pyenv activate test
% pip install .
Processing /Users/joshthoward/Documents/GitHub/trino-python-client
trino requires Python '>=3.6' but the running Python is 3.5.10 |
Awesome! It's a useful error message. I wondered if it was something cryptic and would make the 3.5 users confused about why installs broke. |
This PR includes the following updates: