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

Dependency update #50

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

Guelakais
Copy link

The only dependency I couldn't upgrade straight away was colorgrad. The rest is running now.

Copy link
Member

@carrascomj carrascomj left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

I left some comments, the most important part is that it is panicking for me when I import or export some data.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/data.rs Outdated
Comment on lines 64 to 65
reader.read_to_end(&mut bytes).await.unwrap();
let custom_asset = serde_json::from_slice::<A>(&bytes).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think unwrap is okay here, we don't want to panic and crash the application if there is a malformed JSON.

Probably you need to map the error to make Self::Error

src/info.rs Outdated
1.,
0.,
)))
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This unwrap causes the application to panic the application upon SVG exporting or when drag and dropping a map to the application (maybe on more events).

thread 'main' panicked at src/info.rs:150:14:
called `Result::unwrap()` on an `Err` value: 0.99783385
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `shu::info::pop_infobox`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

(I am testing on --release.)

roarsvg::fill(
roarsvg::Color::new_rgb(fill_color[0], fill_color[1], fill_color[2]),
fill.color.a(),
fill.color.hue(),
Copy link
Member

Choose a reason for hiding this comment

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

Why is the opacity here (and below) set to the hue?

Copy link
Member

@carrascomj carrascomj left a comment

Choose a reason for hiding this comment

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

The issues about crashing the application on some events remain. By the way, for the CI to pass, both cargo fmt and cargo test has to be run without issues (more of this in the docs' contribution section).

src/data.rs Outdated Show resolved Hide resolved
@@ -147,6 +147,6 @@ fn pop_infobox(
1.,
0.,
)))
.unwrap();
.map_err(|e|format!("Error:{}",e.get_represented_type_info().unwrap().type_path())).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This unwrap is not acceptable either.

Removing the unwrap (leaving the error unhandled) makes drag and dropping work but we should investigate why exactly this error arises.

Co-authored-by: Jorge Carrasco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants