-
Notifications
You must be signed in to change notification settings - Fork 121
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
Update treq #401
Update treq #401
Conversation
The latest release of That still fails due to #339. |
twisted/treq#293 requests a Treq release. |
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.
Does this draft require a review?
I am just trying to clean my https://github.com/pulls/review-requested queue :)
@adiroiban I believe that it needs to be changed to reference treq 20.9.0 first. |
@twm updated, but we still have #339. Our options here are (a) fix treq to do what @glyph thinks is correct regarding |
@wsanchez I think that the ideal outcome is to fix Hyperlink. Its current behavior is to corrupt a common sequence in the most common type of URL. I definitely wouldn't have made treq pass all URLs through it if I'd known of that behavior, and it makes Hyperlink totally unsuited for use on the server side, as in Twisted #9359. It seems like a two birds with one stone situation to me. As a fallback, I could make treq avoid using Hyperlink, which has also caused other problems (treq#303). That would fix it in this Klein case, but not help twisted/twisted#970. |
@twm agreed. Like Klein, Hyperlink lacks active developers who will review code, so… if you submit a PR, I can review and merge, then cajole Mahmoud to make a release. |
I'd really like to get it fixed & released in Hyperlink. I'm on vacation (let me pause just a moment to take a sip out of my mug here) so I might have a few spare cycles to help get this over the line in the next couple of weeks. I am working a couple of days, and I am planning to spend the rest of it playing World of Warcraft, but I will try to have a few minutes between raids. |
I gave fixing it in treq a shot. Unfortunately even the raw Meanwhile, both Hyperlink and treq are having CI issues. Hyperlink's issue is Travis (I guess builds pass... eventually?) and treq's is... several things, including Travis and OpenSSL. I filed some PRs:
There are several other treq PRs that need to go in before release, all small. I've started poking at a real fix in Hyperlink, but no promises there. |
@twm Welp, right at the end of vacation, I did manage to squeeze this in: python-hyper/hyperlink#145 |
@glyph I submitted python-hyper/hyperlink#146 to your PR, which also fixes the issue for |
Codecov Report
@@ Coverage Diff @@
## master #401 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 45 45
Lines 3850 3850
Branches 249 249
=======================================
Hits 3794 3794
Misses 39 39
Partials 17 17 Continue to review full report at Codecov.
|
It seems like the build timeouts may be too short? |
It shouldn't take 30 minutes to run the unit tests. I think the problem is something broken with the usage of Hypothesis? I get errors locally about it generating too many tests. (I think that Klein is probably the wrong layer for most of these Hypothesis tests anyway; mostly they're testing HTTP parsing and URL stuff that should be handled by Hyperlink, Twisted, or Hyper H2, I think). |
* master: [requires.io] dependency update [requires.io] dependency update [requires.io] dependency update [requires.io] dependency update Use Hypothesis strategies that are now provided by Hyperlink, remove them from Klein. # Conflicts: # tox.ini
These are moving to Hyperlink, but we do use the strategies, so I need to dig into why sometimes they stall out, but that fix will end up being in Hyperlink. |
OK I've updated Hyperlink and that seems to work without an update to Treq, which we can update out of band from this PR when it releases. |
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.
🎉 |
Update treq, which is necessary to address build failures against Twisted trunk.