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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unwrap from write_png #261

Merged
merged 3 commits into from Feb 27, 2023
Merged

Conversation

Tristramg
Copy link
Collaborator

Unwraps are bad, and we should gradually remove them (#92)

This pull request removes them from a single function to see if we agree on how to do it.

The function now returns a Result (two specific errors have been created) and there still is an expect. So the in case of a failure, the program will still panic.

馃毃 Test instructions

cargo run -p maplibre-demo --features headless -- headless

It write 4 png on the current path

maplibre/src/headless/system.rs Show resolved Hide resolved
maplibre/src/render/error.rs Outdated Show resolved Hide resolved
maplibre/src/render/resource/surface.rs Outdated Show resolved Hide resolved
@maxammann
Copy link
Collaborator

Unwraps are bad, and we should gradually remove them (#92)

Absolutely! This is usually the thing I ignore while doing major refactors because it is trival to do later on in separate clean PRs :)

I'm welcoming the effort!

Ideally we get rid of all .unwrap() and most .expect().

@Tristramg
Copy link
Collaborator Author

Getting rid of expects is for me a nice way to get a bit deeper, and getting feedback on how things are (or should be) organized and can be done with incremental steps

@maxammann
Copy link
Collaborator

I'd actually keep the error even more local: maxammann@1a80067

I'm happy to discuss this because this is a general question which should be answered for this project :)

@Tristramg
Copy link
Collaborator Author

It鈥檚 ok for me. I might need some time to overcome my tendency to define all the errors in one specific place. I am convinced by the argumentation, but it鈥檚 not yet in my muscle memory ;)

I鈥檝e rebased it with you suggestion

@Tristramg Tristramg force-pushed the less_unwrap branch 3 times, most recently from d8dc450 to ceb19ac Compare February 25, 2023 15:56
maxammann
maxammann previously approved these changes Feb 25, 2023
Copy link
Collaborator

@maxammann maxammann left a comment

Choose a reason for hiding this comment

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

Looks good, merging!

@maxammann
Copy link
Collaborator

I just formatted the code again :)

@maxammann maxammann enabled auto-merge (squash) February 25, 2023 23:44
@maxammann maxammann enabled auto-merge (squash) February 25, 2023 23:44
@maxammann maxammann merged commit 5cd3e50 into maplibre:main Feb 27, 2023
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.

None yet

2 participants