search

twitter-archive / cloudhopper-smpp

Efficient, scalable, and flexible Java implementation of the Short Messaging Peer to Peer Protocol (SMPP)
Other
382 stars 356 forks source link

Corrected name of DataCoding 0x01 in SmppConstants to IA5 (not GSM) #107

Closed RobinJDCox closed 9 years ago

RobinJDCox commented 9 years ago

The DataCoding value 0x01 represents IA5, as indicated in the SMPP spec - however the name given to the 0x01 value in SmppConstants is DATA_CODING_GSM not DATA_CODING_IA5 which is misleading.

This changes the name to properly represent the 0x01 DATA_CODING_IA5 value, and sets the DATA_CODING_GSM to the default data coding to prevent breaking existing usages of this constant.

RobinJDCox commented 9 years ago

I see your point with backward compatibility and you're right about the SMSC default alphabet. However, having a constant called GSM that actually points to the IA5 data coding value just seems to be asking for trouble?

RobinJDCox commented 9 years ago

One alternative I suppose would be to simply rename the GSM constant to IA5. Though a breaking change, it would force users to make the choice about whether they actually need the 0x01 or 0x00 data coding value.

Initially I didn't want to do this as it forces users to do make a code change when they update - but maybe that's a good thing in this situation? What do you think?

jjlauer commented 9 years ago

I always defer to backwards compat. Let's simply make GSM point to IA5 and add the deprecated annotation to the GSM constant. Add a comment that we may remove it in a future version.

On Tue, Jun 30, 2015 at 5:19 AM, Robin Cox notifications@github.com wrote:

One alternative I suppose would be to simply rename the GSM constant to IA5. Though a breaking change, it would force users to make the choice about whether they actually need the 0x01 or 0x00 data coding value.

Initially I didn't want to do this as it forces users to do make a code change when they update - but maybe that's a good thing in this situation? What do you think?

— Reply to this email directly or view it on GitHub https://github.com/twitter/cloudhopper-smpp/pull/107#issuecomment-117069976 .

RobinJDCox commented 9 years ago

Have updated my fork making the GSM constant deprecated as suggested