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.

Sign up for GitHub

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

Jump to bottom

add rust lint ignore for match cmp #66

Closed
dpaiton opened this issue May 3, 2024 · 0 comments · Fixed by #113
Closed

add rust lint ignore for match cmp #66

dpaiton opened this issue May 3, 2024 · 0 comments · Fixed by #113
Assignees
@dpaiton
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@dpaiton
Copy link
Contributor

dpaiton commented May 3, 2024

Rust lint (clippy) suggests we make a change from

if x < y
else if x > y
else

to

match x.cmp(y)
ordering.less
ordering.greater
ordering.equal

This is a pretty controversial lint suggestion from clippy. @jalextowle and I talked about it and we both agree that the original formatting is better (more readable, doesn't require extra import). We should set a global ignore for this lint warning.
@dpaiton dpaiton self-assigned this May 3, 2024
@dpaiton dpaiton mentioned this issue May 3, 2024
Merged
6 tasks
@dpaiton dpaiton mentioned this issue May 14, 2024
Merged
6 tasks
@dpaiton dpaiton added bug Something isn't working good first issue Good for newcomers labels May 15, 2024
@dpaiton dpaiton mentioned this issue May 23, 2024
Merged
@dpaiton dpaiton linked a pull request May 24, 2024 that will close this issue
Replace panic with Result #113
Merged
dpaiton added a commit that referenced this issue May 27, 2024
@dpaiton
Replace panic with Result (#113)
2561fae 
# Resolved Issues
#99
#64
#65
#66

# Description
Our use of both panic and result results in a worse developer
experience, and should be changed to using Result everywhere possible.
This PR removes almost all `panic` uses in the core libs; where
functions once would panic but return a type `T` they now return an
error with the type`Result<T>`. I also removed `unwrap()` statements,
which convert `Result` to `panic`, in most places around the repo.

Exceptions were made for core (anything with sugar like `+`, `-`, `*`)
FixedPoint arithmetic (thus keeping any U256 panics), certain
test-related cases, and macros.

I also improved some error messaging (in bindings mostly, but also
elsewhere), added comments, & added a lint ignore.

I ran the fuzz testing with 10k `FUZZ_RUNS` and 50k `FAST_FUZZ_RUNS`
twice without error locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dpaiton dpaiton

Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace panic with Result delvtech/hyperdrive-rs
1 participant
@dpaiton