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

add position to scene errors #8065

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Mar 12, 2023

Objective

Before:
2023-03-12T22:38:59.103220Z  WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected closing `)`
After:
2023-03-12T22:38:59.103220Z  WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected closing `)` at scenes/test/scene.scn.ron:10:4

Solution

Changelog

  • added line numbers to scene errors

@hymm hymm added A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible labels Mar 12, 2023
@james7132 james7132 added this to the 0.10.1 milestone Mar 13, 2023
@MrGVSV MrGVSV added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 20, 2023
.deserialize(&mut deserializer)
.map_err(|e| {
let span_error = deserializer.span_error(e);
anyhow!(
Copy link
Member

Choose a reason for hiding this comment

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

I'd really like for us to reduce our use of anyhow. IMO it's a crate that end-users should be using, not the libraries they're depending on. With that said, this isn't a breaking change from what we're already doing, so I won't block on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was just using anyhow here because it was already the mechanism being used (The ? was converting it to an anyhow Result). Normally for error handling in libs you would have an error enum that would have variants for the possible failure states and that would hold the context needed to display this error. You would construct that variant here instead. Should be easy enough to migrate this to that if/when we make a switch away from anyhow.

@james7132 james7132 added this pull request to the merge queue Mar 20, 2023
@james7132 james7132 merged commit d58ed67 into bevyengine:main Mar 20, 2023
mockersf pushed a commit that referenced this pull request Mar 27, 2023
# Objective

- Fixes #6760
- adds line and position on line info to scene errors

```text
Before:
2023-03-12T22:38:59.103220Z  WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected closing `)`
After:
2023-03-12T22:38:59.103220Z  WARN bevy_asset::asset_server: encountered an error while loading an asset: Expected closing `)` at scenes/test/scene.scn.ron:10:4
```

## Solution

- use span_error to get position info. This is what the ron crate does
internally to get the position info.
https://github.com/ron-rs/ron/blob/562963f88733cae0e3ca2a128b590817f0346343/src/options.rs#L158

## Changelog

- added line numbers to scene errors

---------

Co-authored-by: Paul Hansen <[email protected]>
@hymm hymm deleted the position-in-scene-error branch October 5, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

better error messages when deserializing a scene
5 participants