import fix for signature #23
Conversation
|
Any chance I can get this merged? |
| except ImportError: | ||
| # For python2 capability. NOTE: you must fulfill this requirement | ||
| # yourself, it is not in requirements.txt | ||
| from funcsigs import signature |
There was a problem hiding this comment.
This would be great to cover by tests in TOX or travis matrix with funcsigs explicitly deployed for a python2 environment.
There was a problem hiding this comment.
I agree. If we're adding this new logic, it'd be great to cover it with tests.
sqlalchemy_filters/filters.py
Outdated
| from inspect import signature | ||
| except ImportError: | ||
| # For python2 capability. NOTE: you must fulfill this requirement | ||
| # yourself, it is not in requirements.txt |
There was a problem hiding this comment.
"# yourself, it is not in package requirements" - there is no requirements.txt for this project, install_requires is used instead.
There was a problem hiding this comment.
+1
Also, how would you feel about adding another section to extras_require to support this @iky ? Something like 'python2'.
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras
There was a problem hiding this comment.
Even though there won't be active support for Python 2, I think we should port all the code to use new-style classes if we're adding this change.
I have found one class that could be amended:
class BooleanFilter: to class BooleanFilter(object) in sqlalchemy_filters.filters.py
README.rst
Outdated
| Python 2 | ||
| -------- | ||
|
|
||
| There is no active support for python 2, however it is compatiable as of January 2019, if you install funcsigs. |
There was a problem hiding this comment.
We could replace January with February since this hasn't landed yet on master, even though the PR was created last month.
Maybe we could also add the version when we cut the next release.
| except ImportError: | ||
| # For python2 capability. NOTE: you must fulfill this requirement | ||
| # yourself, it is not in requirements.txt | ||
| from funcsigs import signature |
There was a problem hiding this comment.
I agree. If we're adding this new logic, it'd be great to cover it with tests.
sqlalchemy_filters/filters.py
Outdated
| from inspect import signature | ||
| except ImportError: | ||
| # For python2 capability. NOTE: you must fulfill this requirement | ||
| # yourself, it is not in requirements.txt |
There was a problem hiding this comment.
+1
Also, how would you feel about adding another section to extras_require to support this @iky ? Something like 'python2'.
https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-extras
|
Like they say, 7 times is a charm :) I think I have identified all of the comments. |
There was a problem hiding this comment.
👍 Thanks for the changes @itdependsnetworks
I'm happy to merge it if @iky also approves it.
This addresses https://github.com/Overseas-Student-Living/sqlalchemy-filters/issues/18