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

Convert fallible From implementations to TryFrom #123

Closed
xla opened this issue Jan 5, 2020 · 3 comments
Closed

Convert fallible From implementations to TryFrom #123

xla opened this issue Jan 5, 2020 · 3 comments
Labels
serialization structure High level repo-wide structural concerns

Comments

@xla
Copy link
Contributor

xla commented Jan 5, 2020

Currently there are a handful of cases where the From trait is implemented and the implementation can panic. This is usually addressed with a conversion to a TryFrom implementation. See 71f0cd3 and https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from

@liamsi
Copy link
Member

liamsi commented Jan 6, 2020

Some of these Froms aren't used anywhere. Whoever tackles this might also consider removing some of the impls.

@tarcieri
Copy link
Contributor

tarcieri commented Jan 6, 2020

Which ones? It’d be good to make sure the KMS isn’t using them before removing them.

You can always add a temporary [patch.crates-io] to the KMS’s Cargo.toml pointing it at a local path to double check that.

@ebuchman ebuchman added the structure High level repo-wide structural concerns label Jun 3, 2020
@xla xla removed their assignment Sep 2, 2020
@thanethomson
Copy link
Contributor

Is this still relevant? Looking through the codebase now, I don't see any instances where we're disabling the fallible_impl_from lint, and I think there's one From impl that calls unwrap() internally (converting a u64 to an i64) and there's a TODO above it to fix it.

@xla xla closed this as completed Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
serialization structure High level repo-wide structural concerns
Projects
None yet
Development

No branches or pull requests

5 participants