- add support for multi OPTIONAL_PARAMS hexcode#73
- add support for multi OPTIONAL_PARAMS hexcode#73jasonlazo wants to merge 1 commit intopython-smpplib:masterfrom
Conversation
Some vendors use different hex code so i use a tuple instead of one specific hexcode.
eigenein
left a comment
There was a problem hiding this comment.
Could you also add an item to CHANGELOG.md?
| EIGHTBIT_PART_SIZE = 140 - MULTIPART_HEADER_SIZE | ||
| UCS2_PART_SIZE = 140 - MULTIPART_HEADER_SIZE # must be an even number anyway | ||
|
|
||
|
|
There was a problem hiding this comment.
Could you keep these empty lines that separate different groups of constants?
|
|
||
| for key, value in six.iteritems(consts.OPTIONAL_PARAMS): | ||
| if value == code: | ||
| if code in value: |
There was a problem hiding this comment.
I'd prefer to give it a plural name then:
| if code in value: | |
| for key, values in six.iteritems(consts.OPTIONAL_PARAMS): | |
| if code in values: |
| } | ||
|
|
||
|
|
||
| OPTIONAL_PARAMS = { |
There was a problem hiding this comment.
I would suggest keeping old ids for readability
OPTIONAL_PARAMS = {
key: frozenset({value}) if not isinstance(value, Iterable) else frozenset(value)
for key, value in six.iteritems({
#old values here,
'network_error_code': (0x0423, 0x1403,),
})}There was a problem hiding this comment.
Author's option for me is more explicit, no need to read the conversion code. It's faster to visually skip commas than understand the loop
There was a problem hiding this comment.
Perhaps it is fine. I wouldn't insist
Actually what I was thinking about is to create a reverse of this mapping:
for key, value in six.iteritems(consts.OPTIONAL_PARAMS):
if code in valuecan be probably translated into something like key = REV_OPTIONAL_PARAMS[code]
Some vendors use different hex code so i use a tuple instead of one specific hexcode.