Fix chat.py example #12070
Fix chat.py example #12070
Conversation
Apparently, twisted changed the behavior in that the library now expects byte arrays rather than strings. Also, Python3 replaced the dict method iteritems() with items()
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Thanks for the fix.
It would be nice to have the examples always executed as part of our continous testing ... but this would be a task for another PR.
I am not using IRC with Twisted.
For this PR, I trust that you have executed this example on Py3 and that the updated example works.
As we don't have a maintainer for Twisted IRC support, I think that is best to merge this PR as it is.
Later, if someone cares more about IRC, they can update the code.
| message = f"<{self.name.decode('utf-8')}> {message.decode('utf-8')}".encode( | ||
| "utf-8" | ||
| ) |
There was a problem hiding this comment.
Just a minor comment to avoid all the encode + decode
| message = f"<{self.name.decode('utf-8')}> {message.decode('utf-8')}".encode( | |
| "utf-8" | |
| ) | |
| message = b'<' + self.name + '> ' + message |
There was a problem hiding this comment.
I also agree with this comment but I think
message = b'<' + self.name + b'> ' + messageis correct to avoid TypeError.
Is there any reason for that we didn't applied it?
There was a problem hiding this comment.
Hi. Thanks for the feedback. This was merged as it was better than what we had in trunk. There is alwasy place for improvement :)
Feel free to open a new GitHub Issue and PR if you would like to see this change merged.
The IRC implementation in Twisted needs some love and I would be happy to see people care about this code :)
Cheer
There was a problem hiding this comment.
Dear @adiroiban, I've created an issue and a pr for it.
|
@ralphm if you have time, you can check this. @glyph this is an example where having something like twisted/txirc would help. Ralph could be automatically subscribed to any issue for |
|
@adiroiban While I appreciate that Twisted's IRC implementation is in Words and I probably can review this on its merits, I am not an IRC expert at all. I have primarily contributed to the XMPP related stuff ( If you want to have more granular control over this, we should set up a CODEOWNERS file. |
I wouldn't mind if someone wanted to set up CODEOWNERS to get more granular notifications for things they're interested in, but I emphatically do NOT want to set up fiefdoms and official "owners" within the codebase. We have a limited number of maintainers and by far our most pressing problem is getting people to do more reviews, not preventing 'unqualified' reviewers or requiring an expert for things to get merged. If anyone with commit access feels comfortable, they should feel free to do a review :) |
|
I am also -1 for blocking a PR merge if there is no review from CODEOWNERS. Also, I am not looking for "official" maintainers for parts of twisted/twisted. What I am saying is that I think that it's a bit to ask for twisted/twisted maintainers to know AMP/POP/IMAP/IRC/XMMP or some GPS possitioning protocol. I will wait for @ReneNyffenegger to check my comment. I think that the current PR is an improvemnt over the existing code, so it meets the merge criteria. |
Cool, good to know. If we can use CODEOWNERS that way I'm happy to have it help with making notifications more granular. |
|
CODEOWNERS can work for PR notifications... they will not work for Issues. |
Apparently, twisted changed the behavior in that
the library now expects byte arrays rather than
strings.
Also, Python3 replaced the dict method iteritems() with items()