-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for optional parser parameters #9
add support for optional parser parameters #9
Conversation
Hi! Thanks for working on this :) #7 didn't really talk about this, but more like if an at command has e.g. either 3 or 6 parameters depending on some condition. However, the thing you're implementing here also does happen so it's a good feature to have. |
The way of the refactoring was chosen because if we used the optional methods directly we would already have added the response to the tucat and this would break the API as the non-optional would also return an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean and what you've done here. Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that this fell off my radar a bit. Could you add this to the changelog? Just add an "Unreleased" section to the top of the list and add this PR as a bullet point under it.
Oh that was quick, thanks! Once the other open PR is merged, I'll make a new release :) |
Now released as 0.5.4 |
This should be a discussion starter for an implementation for #7.
It is mostly a copy of the existing methods and tweaking them a bit.
I would also replace the implementation of the none optional counterparts with the optional ones and just wrap it with a None check.