CesiumGS / CesiumGS/cesium-native
Use `Result` for methods that may fail
- Dominant language
- C++
- Stars
- 623
- Forks
- 277
- PR merge metrics
- No merged PRs in 30d
Description
Currently, when we have a method where the operation might fail in some way, we handle this pattern with one of the following approaches:
- Throw an `std::runtime_error` exception.
- Throw an `std::invalid_argument` exception (see #205).
- `reject` the current promise.
- Return an empty `std::optional`.
None of these are ideal approaches. In the first three, unless we have anticipated the exception and wrapped the call in a `try...catch` block, we give up any hope of gracefully handling the failure and just immediately crash. In the fourth approach, which is the one we use most often, we can usually recover from this issue, but we also don't return any information about what caused the error.
A few months ago, @kring added [CesiumUtility::Result](https://cesium.com/learn/cesium-native/ref-doc/structCesiumUtility_1_1Result.html). This is similar to the Rust `Result` type in that it lets us specify an operation that can return one of two results: a valid result, or a failure result (the difference from Rust's `Result` type is that our error type is always an `ErrorList`, not an arbitrary type). This is the "best of both worlds" compared to our current approaches: we can handle the error gracefully like returning `std::nullopt`, but we also can provide error information like when throwing an exception.
`Result` has gotten some use in new code that we've written, but we haven't extended it to the rest of the codebase. It's a breaking change, but I think it would be a breaking change that moves us in the right direction.
Contributor guide
Research direction
No files, tests, or specific methods are named. Start by reading the linked CesiumUtility::Result documentation and inventorying existing failure-handling patterns across the codebase. Done would require an agreed scope for migrating APIs and their callers, plus updated tests, but this issue does not define either.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100