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

feat: argument bounds checking and option filtering #25

Merged
merged 3 commits into from
Jul 14, 2024
Merged

Conversation

cjdjpj
Copy link
Collaborator

@cjdjpj cjdjpj commented Jul 14, 2024

Argument bounds checking: make sure float values for frequencies are valid, panic otherwise.

Option filtering: refactored filtering to use Option type rather than throw an error

Copy link
Owner

Choose a reason for hiding this comment

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

Nice. Thanks. Just keep in mind we'll need to redefine these ErrorKind's into some custom Error type which we can chain in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I am going to do this right after. What do you mean by chaining errors? All I had in mind was a couple (2-4) error structs that encompassed most of the current "errors" and implementing unique display methods for each of them.

Copy link
Owner

Choose a reason for hiding this comment

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

If one error caused another in a cascade, we need to chain them to identify the root cause. I have not looked too hard into implementing this manually, but I believe the anyhow crate implements this. You may use that crate for all the error handling-related stuff or you may implement things yourself from scratch.

@jeffersonfparil jeffersonfparil merged commit 0018676 into main Jul 14, 2024
1 check passed
@jeffersonfparil jeffersonfparil deleted the dev branch July 14, 2024 22:22
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