Conversation
- fixes Makefile to run when you have py2 + py3 installed - removes ClientRedirectError - ran isort based on recommendations from make test
thehesiod
changed the title
|
@asvetlov unfortunately when running the tests locally I'm getting "ValueError: option names {'--fast'} already added" so this may take a few iterations. Let me know what you think. |
|
ah figured it out, I had pytest-aiohttp installed, perhaps should detect/try uninstalling this first before running tests |
|
another strange thing is running isort locally gave me a different result for background_tasks.py than through appveyor. Perhaps needs to be pinned? |
asvetlov
left a comment
asvetlov
left a comment
There was a problem hiding this comment.
The fix is good but I have concern about changes not related to the issue directly.
Makefile
Outdated
| @@ -1,9 +1,11 @@ | |||
| # Some simple testing tasks (sorry, UNIX only). | |||
|
|
|||
| pytest := python3 -m pytest | |||
There was a problem hiding this comment.
What is intention for this variable? It's basically an alias for just pytest, isn't it?
There was a problem hiding this comment.
yes, this is so I could run the tests locally, where I have py2 + py3 installed
There was a problem hiding this comment.
yes, with this change you can run the tests outside virtualenv, still want me revert? it makes no different inside virtual env but fixes it outside virtual env
Makefile
Outdated
|
|
||
| .install-deps: requirements/dev.txt | ||
| @pip install -U -r requirements/dev.txt | ||
| @pip3 install -U -r requirements/dev.txt |
There was a problem hiding this comment.
aiohttp development assumes virtual environment usage.
In proper virtual env pip is just an alias for pip3 if python is python3
| import aiopg.sa | ||
| import sqlalchemy as sa | ||
|
|
||
| import aiopg.sa |
There was a problem hiding this comment.
Please drop this file from PR. The change is not related to issue.
There was a problem hiding this comment.
when I run the tests locally flake said to run isort against this file, still want me to revert?
tox.ini
Outdated
| commands = | ||
| flake8 aiohttp examples tests | ||
| python setup.py check -rm | ||
| python3 setup.py check -rm |
There was a problem hiding this comment.
Again we are in python3 virtual environment here.
Don't need python3 command.
|
Well, your changes about But I'm striving to have relative small PRs for solving one issue per PR. |
Codecov Report
@@ Coverage Diff @@
## master #2030 +/- ##
==========================================
- Coverage 97.05% 97.05% -0.01%
==========================================
Files 38 38
Lines 7700 7697 -3
Branches 1347 1347
==========================================
- Hits 7473 7470 -3
Misses 103 103
Partials 124 124
Continue to review full report at Codecov.
|
|
Last thing: drop |
|
added, btw should adding files to changes/ be documented in CONTRIBUTING.rst? |
|
No, sorry. I very ask you to make a PR for CONTRIBUTING.rst update. |
changes/2030.feature
Outdated
| @@ -0,0 +1,3 @@ | |||
| ClientRedirectError is no longer raised | |||
There was a problem hiding this comment.
As we don't publish aiohttp release with ClientRedirectError the name should be changed to RuntimeError (used to be raised in aiohttp 2.2)
|
Thanks for your patience :) |
|
@thehesiod |
|
@asvetlov no worries, thank you too for the review! |
psf-chronographer
bot
added
the
bot:chronographer:provided
3 participants
resolves #2022
ran isort based on recommendations from make testgave different results