Drop support for botocore <1.11.0 and requests <2.16.2 (fixes #693) #698
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #698 +/- ##
==========================================
+ Coverage 89.74% 90.03% +0.28%
==========================================
Files 27 27
Lines 1746 1726 -20
Branches 311 308 -3
==========================================
- Hits 1567 1554 -13
+ Misses 144 137 -7
Partials 35 35
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
approve this! |
vcr/patch.py
Outdated
| try: | ||
| import botocore.vendored.requests.packages.urllib3.connectionpool as cpool | ||
| import botocore.vendored.requests # noqa: F401 | ||
|
|
||
| raise RuntimeError( | ||
| "vcrpy >=4.2.2 and botocore <1.11.0 are not compatible" | ||
| "; please upgrade botocore (or downgrade vcrpy)" | ||
| ) | ||
| except ImportError: # pragma: no cover | ||
| pass |
There was a problem hiding this comment.
Can you also remove this try/except and the vendored import? If the initial import failed raising the RuntimeError should be enough?
Additionally, should we use raise RuntimeError(...) from e, e being the initial ImportError?
There was a problem hiding this comment.
Can you also remove this try/except and the censored import? If the initial import failed raising the RuntimeError should be enough?
@pquentin I'm not sure I get the idea. If we can cut anything away here, I'll need your help to see it. Could you elaborate?
Additionally, should we use
raise RuntimeError(...) from e, e being the initial ImportError?
To my understand that from e is happening automatically and would need suppressing via from None if we wanted the opposite. To see it for yourself in practice (I tried Python 3.8), you could run these two programs:
Nested traceback output:
try:
raise ValueError('first')
except:
raise ValueError('second')Proof of set __context__:
try:
try:
raise ValueError('first')
except:
raise ValueError('second')
except Exception as e:
print(repr(e.__context__))What do you think?
There was a problem hiding this comment.
@pquentin PS: quick update, I found one small difference when adding from e1 to the above sample — the message in between the two tracebracks changes slightly:
- a) Default:
During handling of the above exception, another exception occurred: - b)
from e1:The above exception was the direct cause of the following exception:
For myself, the difference is of little value. Do you see value in the difference?
There was a problem hiding this comment.
My message was not clear, because my phone autocorrected from vendored to censored. Anyway, I understand how we need the try/except because requests is an optional dependency.
Do we need import botocore.vendored.requests though?
@pquentin PS: quick update, I found one small difference when adding from e1 to the above sample — the message in between the two tracebracks changes slightly: [...]
Yes, I think this is valuable, because in case a) you don't know if the exception handling failed or if the new exception was intentional.
There was a problem hiding this comment.
My message was not clear, because my phone autocorrected from vendored to censored. Anyway, I understand how we need the try/except because requests is an optional dependency.
@pquentin cool!
Do we need
import botocore.vendored.requeststhough?
I believe we do, because how else would we know that we are dealing with botocore <1.11.0 without that import? In theory we could (a) use other means of detection or (b) not (be able to) throw the runtime error or (c) keep as is. What do you think?
Yes, I think this is valuable, because in case a) you don't know if the exception handling failed or if the new exception was intentional.
I have added it now just for you 😄
There was a problem hiding this comment.
Ah yes because botocore is optional so you need to handle the case where it is not here at all! Makes sense.
Maybe contextlib.suppress would have been slightly clearer and would have avoided the nocover pragma.
Anyway, thanks, you can consider this comment resolved! Thanks for taking the time to explain things.
There was a problem hiding this comment.
@pquentin while I like contextlib.suppress in general, to work here the raise RuntimeError would need to go into the with body and and I think that would make it a bit mind bending. but with that in mind I now have refactored it to pull the raise RuntimeError into a new else branch to hopefully be more obvious and more consistent to the rest of it. I hope that's good with you. Thanks for the review!
@pquentin good point, done, pushed. |
|
@hartwork This looks much easier to maintain going forward :) Thank you. |
|
I'll dare merge this so that we can move forward with urllib3 2.x… |
|
Hi @hartwork / @jairhenrique, it's great to see the urllib3 bug fixed -- when do you plan to release the new version? Thanks! |
|
@randomir this does not fix urllib3 2.0 compatibility yet - it's an intermediate step to make adding support easier! |
|
@pquentin, oh, I see. I got the wrong impression then from this comment, and haven't bothered double-checking. Anyway, do you have an approximate ETA for the urllib3 issue fix? It's quite annoying, but I'm hesitant to patch it with an upper-bound on urllib3... Thanks! |
|
sorry for confusion - it seemed like shifqu's work was more complete and in the correct direction than what I had put together in a slapdash attempt. |
|
Thank you, @hartwork, I'll keep my 👀 open. 😄 |
Fixes #693
CC @jairhenrique @pquentin @vEpiphyte