SMPP needs to have a pluggable codec class.

[smn]

It is possible for MNOs to give us messages that are in an encoding that aren't in the standard encodings that Python ships with.

We have two options:

1. Register a new codec with the codec registry.
2. Create a pluggable codec class for SMPP that fallbacks to python's codec registry for things it knows about but provides hooks for providing other encodings.

We've chosen to go with option 2 because adding a codec to the registry introduces all sorts of potential code loading race conditions.

[smn]

This would solve #338 for SMPP.

[smn]

@rudigiesler @justinvdm @hodgestar can I get a review?

[rudigiesler]

👍, but I don't know the Vumi code-base well enough to know if this might mess with something else.

[justinvdm]

We implemented a ucs2 codec in this PR that proxies to the utf-16be codec. Should we maybe refer to it here?

[smn]

Yeah, my thinking was to have the map in the DeliveryShortMessageProcessor be only codecs that already exist in the standard python distribution. The configurable codec_class can override some of these if it wants, which it does with the ucs2 implementation it provides.

I'm ±0 on this really, it seemed a sensible thing to do yesterday but happy to stick in a custom codec here as well.

@hodgestar thoughts?

[justinvdm]

Ah, ok, that works.

[hodgestar]

I think sticking to the built-in codecs by default makes sense for now -- we can shift things around easily later.

[justinvdm]

Minor comment, otherwise looks good.

[justinvdm]

👍

[smn]

@rudigiesler thanks for the review, I don't expect you to know everything or be catching problems. The best way to learn is by reading PRs (which is why we're constantly asking you to review stuff).

[hodgestar]

Seem like performance on this would be terrible. Should we construct a reverse mapping?

[smn]

Good idea, let me start on that.

[hodgestar]

\o/

[hodgestar]

I left a bunch of questions but they're mostly just to check my understanding of the changes.

[hodgestar]

I'm wondering whether we should land this and then sort out the 7-bit packing in a separate PR? We should start that PR by adding an integration test that sends in a 7-bit packed PDU and checks that we handle it correctly?

👍 on this PR landing once we have the ticket for the new one.

[smn]

@hodgestar GSM7Bit now returns bytestrings, I'm reasonably convinced that I saw that that is what we were receiving anyway.

Also added handling of errors kwarg somewhat properly.

[hodgestar]

Does this perhaps need to be return self.gsm_basic_charset_map.get('?')?

[hodgestar]

I guess we should have tests for the error cases?

[smn]

good catch!

[smn]

@hodgestar ready for rereview

[smn]

@rudigiesler @justinvdm also again ready for re-revieww

[hodgestar]

I noticed another issue here -- we call the same error handlers for both encoding and decoding, but I don't think that makes sense:

• For decoding, handle_replace_error needs to return u'?'.
• For decoding and encoding, handle_strict_error should raise UnicodeDecodeError or UnicodeEncodeError as appropriate.

[smn]

I had a feeling this was a bad idea to begin with.

[smn]

@hodgestar ok, again :) Not entirely happy with some of the duplication but it's not too bad.

[hodgestar]

This should check that UnicodeEncodeError is raised (and the equivalent decode test should have a similar change).

[smn]

thanks, done.

[hodgestar]

Other than one small comment, looking good.

[hodgestar]

👍 as soon as a Travis build passes (looks like they're building now).