Pin to jsonschema<4 and update config for testing on recent Python versions #385
Conversation
|
From #383 (comment)
|
| - uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - run: pip install tox | ||
| - run: | | ||
| tox -e py$(tr -d "." <<<"${{ matrix.python-version }}") |
There was a problem hiding this comment.
This will properly install and then test with the right Python versions
There was a problem hiding this comment.
Small tip: You could install tox-gh-actions to let the system call the correct tox environment. This will avoid as well the usage of shell-expansions which might be different if tests are run on different OS[1].
You can see it in use in macisamuele/language-formatters-pre-commit-hooks
Example of run here
[1] Actually github actions does support running tests on MacOs and Windows, so it might be nice to centralise testing CI into a single provider (instead of depending on appveyor)
| @@ -54,7 +54,7 @@ def wrapper(value): | |||
| return value | |||
| return func(value) | |||
|
|
|||
| return wrapper | |||
| return wrapper # type: ignore | |||
There was a problem hiding this comment.
Sadly the flak8 installed by pre-commit in Python2 does not support specifically ignoring types. This was # type: ignore[return-value] since wrapper is a callable but it's not being coerced to FuncType by mypy even though it should be equivalent (there's probably something more specific that's going on, but it's not particularly obvious).
| pytest-benchmark[histogram] | ||
| pytest-cov | ||
| pytest<4.7 # need support for Python 2.7, see https://docs.pytest.org/en/latest/py27-py34-deprecation.html |
There was a problem hiding this comment.
This breaks for Python 3.10 will automatically be handled by pip for Python 2.7. Mypy however needs pytest<6 but that and the other mypy specific packages have been moved to requirements-typing.txt
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| install_requires = [ | |||
| "jsonref", | |||
| "jsonschema[format]>=2.5.1", | |||
| "jsonschema[format]>=2.5.1,<4.0.0", | |||
There was a problem hiding this comment.
The original point of this PR 😅
terencehonles
force-pushed
the
pin-jsonschema-and-update-config
branch
from
53d9edb to
e6a31dd
Compare
|
I bumped AppVeyor to a config which actually has Python 3.9, but I still can't reproduce the Python2 error as mentioned in #383 (comment) (but now it's at least the same point version so it does seem like this is a Windows only issue) |
- To report more than `<generator object ...>` - To properly exclude `str` and `unicode` on Python3 and Python2 respectively. - To exclude `bool` and `bytes` which are also immutable - To log types of the offending objects in case they need to be special cased in the future.
terencehonles
force-pushed
the
pin-jsonschema-and-update-config
branch
from
ab481f2 to
9f1280b
Compare
|
It looks like it was an issue with |
There was a problem hiding this comment.
changes looks good to me but I'm not using the library by sometime so I'm not fully aware of what are the needs for jsonschema and pytest dependencies and what implications could have, with respect to the Yelp internal build system, the split of the dependencies from dev and typing.
Considering the above, I would delegate @analogue for the final review.
| - uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - run: pip install tox | ||
| - run: | | ||
| tox -e py$(tr -d "." <<<"${{ matrix.python-version }}") |
There was a problem hiding this comment.
Small tip: You could install tox-gh-actions to let the system call the correct tox environment. This will avoid as well the usage of shell-expansions which might be different if tests are run on different OS[1].
You can see it in use in macisamuele/language-formatters-pre-commit-hooks
Example of run here
[1] Actually github actions does support running tests on MacOs and Windows, so it might be nice to centralise testing CI into a single provider (instead of depending on appveyor)
| @@ -2,15 +2,33 @@ | |||
| name: build | |||
| on: push | |||
| jobs: | |||
| pytest: | |||
There was a problem hiding this comment.
| pytest: | |
| tests: |
This combines #383 and #384 with some extra cleanup to get github actions working properly (They seem to be disabled here, but I have them now passing on my fork https://github.com/terencehonles/bravado-core/actions/runs/1358474617)