Skip to content

Conversation

@Masterchef365
Copy link
Contributor

For my use-case, I'm not guaranteed to be provided a solvable matrix; instead of panicking inside rsparse when this happens, I want to be able to handle the error. This PR is a crude draft; all I did was replace panic!() with Result<(), String>. If this feature is wanted, I can convert the String into a thiserror enum, which would be easier for libraries to consume. Open to suggestions!

@RLado
Copy link
Owner

RLado commented Feb 17, 2025

Thanks for the proposal. I think you are right, it is generally a bad idea to panic on a library that other developers are expected to use. Changing those panic! for Result is the better way to handle those situations. I'll be happy to review your PR.

@Masterchef365 Masterchef365 marked this pull request as ready for review February 17, 2025 19:52
@Masterchef365
Copy link
Contributor Author

Okay, I've switched the String to be an enum which implements std::error::Error, that should be nicer for library users to consume

Copy link
Owner

@RLado RLado left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the PR

@RLado RLado merged commit 494a3fb into RLado:master Feb 20, 2025
1 check passed
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