-
Notifications
You must be signed in to change notification settings - Fork 30
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
FIX: guess_stream_needs_encoding must return False when fileobj is an instance of io.TextIOBase #185
base: master
Are you sure you want to change the base?
Conversation
… instance of io.TextIOBase, because fileobj.write needs a str as parameter, not bytes
rgd https://github.com/crossbario/txaio/actions/runs/7251555425/job/19755043215?pr=185#step:4:202 it would make sense to upgrade CI txaio/.github/workflows/main.yml Line 39 in 50a111a
for testing against latest PyPy 3.9 and 3.10 instead of 3.7 / 3.9 |
thanks for bumping pypy! now it runs into https://github.com/crossbario/txaio/actions/runs/7254104155/job/19763863038?pr=185#step:6:307 I'm not sure what's going on .. this seems to indeed change some deep-buried behavior @meejah any clues or hints or caveats you see? |
the test failure appears also on main branch git checkout master |
it works with last twisted version 23.10.0, it only fails with the Twisted trunk |
this package does work as is https://github.com/crossbario/txaio/actions?query=workflow%3Amain what's not verified if it works with only moving to newer cpython/pypy still, and with current twisted trunk so if you want this going forward, best would be 2 PRs: one with only the cpy/pypy bumping. I also assume setup.py needs adjustment to account for those new minimal version if that works (on 1st PR), then I would relook into your actual change on a 2nd PR if that 2nd PR does work, but does not work on twisted trunk, this must be investigated first, because this is quite unusual - but of course not impossible sorry, I don't have time to work on this, hope above helps nevertheless |
Sorry for the delay, rolled off my radar (ahem, github notifications ;) Just looking at errors + code, it seems like possibly the Bigger picture, a bunch of this seems related to Python2 support -- if that's dropped (it is, right?), I think it could be simplified significantly in all likelihood. |
thanks @meejah for chiming in and for your analysis & comments!
yes, Python 2 is dead and yes, if it would simplify things, we could definitely rip out anything that originally was only to make Python 2 fly from txaio (or autobahn or crossbar for that matter) |
FIX: guess_stream_needs_encoding must return False when fileobj is an instance of io.TextIOBase
because fileobj.write needs a str as parameter, not bytes