You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Option.unwrap() and Result.unwrap() are great tools for Moving fast and breaking things, but are often tested with a specific environment and can cause unexpected issues at runtime.
I count 700+ unwraps in the codebase. I'm sure plenty of these are OK and the developer verified that it'll never panic. In many other cases though, like the ones we've submitted PR's for, only panicked because something was different about our environment, such as a different browser/platform, or not using a feature like manganis.
In a perfect world a library should never panic, Rust has many constructs to avoid doing so, and having some sort of default behavior like in my PR #2675 is often the best way to handle things. My PR essentially changes the meaning of the code from: Panic if not using manganis to Return no files, because the user is not using manganis.
I propose a GitHub workflow be added for the pull_request event. It could run a simple script that checks if the changed code contains any calls to unwrap() (or friends). If it finds any, it could require a maintainer to indicate/approve that the unwraps will never panic (I'm not sure if GitHub workflows has a feature that requires a maintainer approve something like this, as I've never used workflows personally).
Another idea would be to add a CONTRIBUTING.md to the repo, that says unwrap (and friends) should be avoided unless there's no chance of them panicking.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
Option.unwrap()
andResult.unwrap()
are great tools for Moving fast and breaking things, but are often tested with a specific environment and can cause unexpected issues at runtime.I've been working on a project with a friend and we've ran into several random panics while using the latest
main
version of Dioxus. Here are some PR's we've submitted:Handle deserialize errors without panic
Don't panic when not using manganis
I count 700+ unwraps in the codebase. I'm sure plenty of these are OK and the developer verified that it'll never panic. In many other cases though, like the ones we've submitted PR's for, only panicked because something was different about our environment, such as a different browser/platform, or not using a feature like manganis.
In a perfect world a library should never panic, Rust has many constructs to avoid doing so, and having some sort of default behavior like in my PR #2675 is often the best way to handle things. My PR essentially changes the meaning of the code from:
Panic if not using manganis
toReturn no files, because the user is not using manganis
.I propose a GitHub workflow be added for the pull_request event. It could run a simple script that checks if the changed code contains any calls to
unwrap()
(or friends). If it finds any, it could require a maintainer to indicate/approve that the unwraps will never panic (I'm not sure if GitHub workflows has a feature that requires a maintainer approve something like this, as I've never used workflows personally).Another idea would be to add a CONTRIBUTING.md to the repo, that says
unwrap
(and friends) should be avoided unless there's no chance of them panicking.Beta Was this translation helpful? Give feedback.
All reactions