Skip to content

Allow defining default generator's first sequence#91

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

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

Conversation

@Lynesth

@Lynesth Lynesth commented Aug 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

eigenein commented Aug 2, 2019

Copy link
Copy Markdown
Collaborator

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

Lynesth commented Aug 2, 2019

Copy link
Copy Markdown
Contributor Author

@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
View reviewed changes

@eigenein eigenein left a comment

Copy link
Copy Markdown
Collaborator

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

code-of-kpp commented Aug 3, 2019

Copy link
Copy Markdown
Member

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

@Lynesth

Lynesth commented Aug 4, 2019

Copy link
Copy Markdown
Contributor Author

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

@code-of-kpp

code-of-kpp commented Aug 4, 2019

Copy link
Copy Markdown
Member

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

@code-of-kpp

code-of-kpp commented Aug 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

@code-of-kpp

code-of-kpp commented Aug 4, 2019

Copy link
Copy Markdown
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

eigenein commented Aug 4, 2019

Copy link
Copy Markdown
Collaborator

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

@code-of-kpp

code-of-kpp commented Aug 4, 2019

Copy link
Copy Markdown
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

Lynesth commented Aug 5, 2019

Copy link
Copy Markdown
Contributor Author

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

code-of-kpp
code-of-kpp reviewed Aug 6, 2019
View reviewed changes
Comment thread README.md
import mymodule

generator = mymodule.PersistentSequenceGenerator()
generator = mymodule.MyAwesomeSequenceGenerator()

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

Copy link
Copy Markdown
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

@code-of-kpp code-of-kpp Aug 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@Lynesth Lynesth Aug 6, 2019

Copy link
Copy Markdown
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

Reviewers

@code-of-kpp code-of-kpp code-of-kpp approved these changes
@eigenein eigenein eigenein left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Lynesth @eigenein @code-of-kpp