maplibre / maplibre-rs

Experimental Maps for Web, Mobile and Desktop
Apache License 2.0
1.34k stars 77 forks source link

Remove unwrap from write_png #261

Closed Tristramg closed 1 year ago

Tristramg commented 1 year ago

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

maxammann commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I'd actually keep the error even more local: https://github.com/maxammann/maplibre-rs/commit/1a80067ffef104240305a5872cbcbba100126c7a

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

Tristramg commented 1 year ago

It’s 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’s not yet in my muscle memory ;)

I’ve rebased it with you suggestion

maxammann commented 1 year ago

I just formatted the code again :)