Skip to content

Function to parse short_message from deliver_sm#90

Open
Lynesth wants to merge 2 commits intopython-smpplib:masterfrom
Lynesth:patch-4
Open

Function to parse short_message from deliver_sm#90
Lynesth wants to merge 2 commits intopython-smpplib:masterfrom
Lynesth:patch-4

Conversation

@Lynesth
Copy link
Contributor

@Lynesth Lynesth commented Jul 31, 2019

Not sure if needed ot if correctly implemented but I've added a function to parse the short_message property of pdus from deliver_sm commands since not all vendors fill in optional parameters like receipted_message_id.

Example:

# Inside the message_received_handler
pdu.parse_deliver_receipt()

This returns a dict like this one:

{
    'dlvrd': b'000',
    'done_date': b'1907311208',
    'err': b'001',
    'id': b'6C7C7DE28BB3E961947700000000FE37',
    'stat': b'UNDELIV',
    'sub': b'001',
    'submit_date': b'1907311208',
    'text': b'This is a test.'
}

Since it depends on a regex and all vendor surely don't implement the receipt the same way (according to the specs: The format for this Delivery Receipt message is SMSC vendor specific but following is a typical example of Delivery Receipt report.) I've added a way to set the regex manually:

smpp.pdu.set_delivey_receipt_regex(br'myawesomeregex')

Note that the regex must be bytes.

@Lynesth
Copy link
Contributor Author

Lynesth commented Jul 31, 2019

Oh this fails on 2.7 because of the rb in front of the regex. Not sure how to fix this right now, will look at it later.

@eigenein
Copy link
Collaborator

eigenein commented Jul 31, 2019

Try br instead:

Python 2.7.10 (default, Feb 22 2019, 21:55:15) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.37.14)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> br'\n'
'\\n'

@Lynesth
Copy link
Contributor Author

Lynesth commented Jul 31, 2019

@eigenein Thanks that did the trick.

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 5, 2019

@podshumok @eigenein Could I get some review on this when you have the time ?
I am using this in my implementation at the moment because my SMSC doesn't fill optional parameters in deliver_sm so I think it could be of use to other people as well ?

@eigenein
Copy link
Collaborator

eigenein commented Aug 6, 2019

I'm a bit 50/50, I mean, it looks good and helpful, but I'd make a utility function outside of PDU class. My motivation is that this is rather not a PDU feature but a kind of "helper" to parse a vendor-specific status short_message.

@podshumok Did you have a chance to look at this?

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 6, 2019

@eigenein Even though it is vendor specific it seems to be pretty widely used as is and most of the providers use that format.
If I understood correctly you would prefer if it was out of the lib ?

Copy link
Member

@code-of-kpp code-of-kpp left a comment

Choose a reason for hiding this comment

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

I don't think that it should live in PDU class, especially the part with changing class variable in statimethod.

It is OK as external function. It would be nice to provide simple integration with Client.set_message_received_handler

command = None
status = None
_sequence = None
dlr_regex = None
Copy link
Member

Choose a reason for hiding this comment

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

please, avoid abbreviations


return desc

@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

What If I work with two SMSCs with different deliver_sm message formats?

raise ValueError("Invalid PDU command: %s", self.command)

if PDU.dlr_regex is None:
PDU.dlr_regex = re.compile(br'^id:(?P<id>\S+)\s+sub:(?P<sub>\S+)\s+dlvrd:(?P<dlvrd>\S+)\s+submit date:(?P<submit_date>\S+)\s+done date:(?P<done_date>\S+)\s+stat:(?P<stat>\S+)\s+err:(?P<err>\S+)\s+Text:(?P<text>.*)$', re.IGNORECASE)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep lines at 79 symbols (PEP8). You can split

(
    br'your'
    br' string'  # with some comments
    ' like' br'this'
)

for example

Copy link
Member

Choose a reason for hiding this comment

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

this default pattern can live in consts

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 6, 2019

Feel free to close it if you want. I won't have time to modify it anytime soon and wouldn't know where to put it anyway.

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.

3 participants