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: remove anyerror and apply clippy fixes #23

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

sno2
Copy link
Contributor

@sno2 sno2 commented Oct 22, 2024

First, this removes the anyerror dependency and instead uses thiserror to expose parsing errors. Secondly, this makes many getx() and setx() functions return an Option instead of a Result<T, FooErr>. The main reason for this change is that it follows the std's convention (e.g. Vec::get). Finally, this patch removes all Result return types that do not ever return an error.

First, this removes the anyerror dependency and instead uses thiserror
to expose parsing errors. Secondly, this makes many getx() and setx()
functions return an Option<T> instead of a Result<T, FooErr>. The main
reason for this change is that it follows the std's convention
(e.g. Vec<T>::get). Finally, this patch removes all Result<T> return
types that do not ever return an error.
@sriram98v
Copy link
Owner

This looks like a really helpful change! I think that using results for tree edit operations is still necessary however, as I would like an error to be thrown if the edit operation is infeasible.

@sno2
Copy link
Contributor Author

sno2 commented Oct 25, 2024

Which edit operations should be able to return an Err(_)? I noticed that there are a few asserts (e.g. https://github.com/sriram98v/phylo-rs/pull/23/files#diff-9794019b962b882943346745baf52516c9f655c4198f992f64bfdb6bd545f9d2R738-R742) but these would panic and not return an error.

@sriram98v
Copy link
Owner

sriram98v commented Oct 25, 2024

Right, never mind! I meant to add errors to the SPR, NNI, and TBR traits, but I haven't gotten to it yet; I will add those soon. I would say leave those traits unchanged for the time being (SPR, NNI, and TBR); the other changes in this pull request look fine to me. I will send a "request change" to this PR

Copy link
Owner

@sriram98v sriram98v left a comment

Choose a reason for hiding this comment

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

Please leave SPR, NNI, and TBR return types unchanged. the others look fine to me

sno2 added 2 commits October 27, 2024 00:23
Created by running
```
cargo clippy --all-targets --all-features --fix
```
@sno2 sno2 changed the title chore: remove anyerror and unreachable Result<T> returns feat: remove anyerror and apply clippy fixes Oct 27, 2024
@sriram98v sriram98v merged commit 29eb851 into sriram98v:main Oct 28, 2024
1 check passed
@sno2 sno2 deleted the no-anyerror branch October 28, 2024 19:52
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