fiorix / go-smpp

SMPP 3.4 Protocol for the Go programming language
MIT License
220 stars 134 forks source link

Initial implementation of submit_multi PDU #33

Closed VDVsx closed 8 years ago

VDVsx commented 8 years ago

Tested with SMPPSim simulator, tried to not change the API much to avoid more repetition, so SM can optionally hold submit multi parameters, but any improvement suggestions welcome. Once approach is validate I'll add more code to validate size of addresses and distribution lists as per SMPP spec.

VDVsx commented 8 years ago

@fiorix could you please take a look ? thx

fiorix commented 8 years ago

Looks ok for the most part. I'm just unsure about the ShortMessage struct having mutually exclusive fields Dst vs these new lists. Also, have you checked for code coverage? I think the UT you added does not cover failure scenarios.

VDVsx commented 8 years ago

No didn't check code coverage, will do. I'm also a bit in the fence regarding this new fields, this way we could use same API, if we create new struct just for the multi sending, some fields would need to be repeated. Another option is to wrap these new fields inside a new struct that would live inside the ShortMessage, was the later what you mean above ?

fiorix commented 8 years ago

I was thinking to have all separate. But I get that it will lead to a lot of code repetition.

VDVsx commented 8 years ago

Checked and improved the coverage now regarding the added code (before pr -> after pr): smpp: 82.7% -> 83.1% pdufield: 72,5% -> 80.7%

VDVsx commented 8 years ago

@fiorix so how should we proceed with this one ?

fiorix commented 8 years ago

I was about to click Merge but then I have one more question: will this also work for long messages? e.g. long message with dest list.

VDVsx commented 8 years ago

No, SubmitLongMsg() has own implementation I didn't change that one, so those new fields will be ignored in that case. SubmitLongMsg and Submit() also share a lot of common code, first step would be to merge those/create new function with shared code.

VDVsx commented 8 years ago

ping ?

fiorix commented 8 years ago

Thanks! 👍