Skip to content

Set encoding strings as properties to allow override#131

Closed
ricrogz wants to merge 2 commits intojaraco:masterfrom
ricrogz:master
Closed

Set encoding strings as properties to allow override#131
ricrogz wants to merge 2 commits intojaraco:masterfrom
ricrogz:master

Conversation

@ricrogz
Copy link
Copy Markdown

@ricrogz ricrogz commented Oct 29, 2017

Decoding of incoming messages could be handled as mentioned in the documentation.

But encoding of client's outgoing messages is hardcoded to 'utf-8'. To allow for compatibility with servers configured to work with other encodings, it should be possible to use other encodings. Therefore, I suggest to set the encoding as a property that can be overridden.

Copy link
Copy Markdown
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for describing your use-case. I'm rather surprised there are still servers that are not yet supporting UTF-8 encoding, but alas we live in a diverse world, so perhaps it makes sense to support it in the library.

Rather than storing properties on the classes, I think a better approach would be to provide a method for handling the encoding that may be overridden by a subclass. I'll present that in an alternative approach.

Comment thread irc/client.py Outdated

def __init__(self, reactor):
super(ServerConnection, self).__init__(reactor)
self.outgoing_encoding = 'utf-8'
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this behavior is repeated in another Connection subclass, the behavior should probably exist in the superclass.

I'd prefer the name transmit_encoding.

Comment thread irc/server.py
self.buffer.feed(data)
for line in self.buffer:
line = line.decode('utf-8')
line = line.decode(self.incoming_encoding)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I prefer to keep the hard-coded value. I'm not aware of any use case that would demand any other encoding.

@jaraco
Copy link
Copy Markdown
Owner

jaraco commented Oct 29, 2017

Consider 4e02303, which I think serves the use-case you presented. By exposing the value as a class attribute, it also enables other ways to override encoding:

class MyConnection(ServerConnection):
    transmit_encoding = 'latin-1'

or

class MyComplexEncodingConnection(ServerConnection):
    def encode(self, msg):
        return msg.encode('utf-8') if random.randint(1,2) == 1 else msg.encode('latin-1')

But of course, you can still simply set the transmit_encoding attribute on an instance of the class and all messages transmitted subsequently will use that encoding:

conn = ServerConnection()
conn.transmit_encoding = 'latin-1'

Let me know if that works for you and I'll cut a release, or if it doesn't how it could be improved.

@ricrogz
Copy link
Copy Markdown
Author

ricrogz commented Oct 29, 2017

Works for me, thanks!

@ricrogz ricrogz closed this Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants