Skip to content

Allow defining default generator's first sequence#91

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

Allow defining default generator's first sequence#91
Lynesth wants to merge 5 commits intopython-smpplib:masterfrom
Lynesth:patch-5

Conversation

@Lynesth
Copy link
Contributor

@Lynesth Lynesth commented Aug 1, 2019

That way anyone can easily implement a "persistent" generator without having to create a new class.
Feel free to comment whatever you think about it.

client = smpplib.client.Client('example.com', SOMEPORTNUMBER, start_sequence=0x00012345)

Should I add some checks to make sure it is a int that is in the valid range 0x00000000-0x7FFFFFFF ?

@eigenein
Copy link
Collaborator

eigenein commented Aug 2, 2019

Should I add some checks to make sure it is a int that is in the valid range 0x00000000-0x7FFFFFFF ?

If you're up to, otherwise just merge it as is

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 2, 2019

@eigenein Something like this ?
I wanted to make custom exceptions first but then I thought this was generic enough to use the built-ins.

eigenein
eigenein previously approved these changes Aug 2, 2019
Copy link
Collaborator

@eigenein eigenein left a comment

Choose a reason for hiding this comment

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

Yep, I think TypeError and ValueError is a good enough choice 👍

@code-of-kpp
Copy link
Member

Please, add/update requirements for custom sequence generator to README as potentially breaking changes

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 4, 2019

@podshumok I understand the part about adding information about it on the README but why are you mentionning breaking changes ?

@code-of-kpp
Copy link
Member

I take that back, I thought you create other implementation instance if it is passed

@code-of-kpp
Copy link
Member

code-of-kpp commented Aug 4, 2019

confusing part is that start_sequence will not work with custom sequence generator

@code-of-kpp
Copy link
Member

So I think we don't need this argument in Client constructor. You can create SimpleSequenceGgenerator instance with desired start_sequence and pass this instance to the client's __init__

@eigenein
Copy link
Collaborator

eigenein commented Aug 4, 2019

I'm okay with either option. One seems less DRY, another is more friendly

@code-of-kpp
Copy link
Member

I vote for SimpleSequenceGenerator, ask for some documentation and/or tests (but I don't insist), and vote against start_sequence in Client

@Lynesth
Copy link
Contributor Author

Lynesth commented Aug 5, 2019

@podshumok @eigenein Made some changes, let me know if that's ok now.

import mymodule

generator = mymodule.PersistentSequenceGenerator()
generator = mymodule.MyAwesomeSequenceGenerator()
Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate a little about the interface such a Generator has to have

Copy link
Member

@code-of-kpp code-of-kpp Aug 6, 2019

Choose a reason for hiding this comment

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

If you don't have time for this, also please let us know and @eigenein will merge it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@podshumok @eigenein Feel free to add this info to the readme if you wish, I don't think I will have time to.

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