Skip to content
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

Merged
merged 3 commits into from
Sep 16, 2023

Conversation

tarfu
Copy link
Contributor

@tarfu tarfu commented Sep 3, 2023

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.

@diondokter
Copy link
Owner

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.
One thing though is that I don't really like the amount of duplicated code.
It would be nice if it could be refactored so that the non-optional functions would simply use the optional functions.
That way going forward the maintenance will be easier.

@tarfu
Copy link
Contributor Author

tarfu commented Sep 6, 2023

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 Option.

@tarfu tarfu marked this pull request as ready for review September 9, 2023 15:54
Copy link
Owner

@diondokter diondokter left a 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!

Copy link
Owner

@diondokter diondokter left a 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.

@diondokter
Copy link
Owner

Oh that was quick, thanks!

Once the other open PR is merged, I'll make a new release :)

@diondokter diondokter merged commit db25b5b into diondokter:master Sep 16, 2023
4 checks passed
@diondokter
Copy link
Owner

Now released as 0.5.4

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.

2 participants