search

georust / geozero

Zero-Copy reading and writing of geospatial data.
Apache License 2.0
352 stars 37 forks source link

Refactor MVT and GDAL errors #148

Closed edpft closed 1 year ago

edpft commented 1 year ago

Hi @nyurik, I'm keen to contribute a PR but I want to check I'm heading down the right track. Please let me know if this was the sort of thing you were thinking of!

So far, all I've done is add an error module to the MVT module.

Related: #138

nyurik commented 1 year ago

BTW, because of your PR, I created a clippy issue https://github.com/rust-lang/rust-clippy/issues/10902

pka commented 1 year ago

@edpft great improvement of my poor error enums! Thanks for your contribution and thanks @nyurik for reviewing!

edpft commented 1 year ago

Hi @nyurik, I've replaced the stringy errors with proper enums but I haven't fully implemented the module level error types that you suggested in the original issue. That's possible but it would be a major refactor as all the current top-level errors would need to be replaced. I can go down that route but it'll take a while!

nyurik commented 1 year ago

I created a https://github.com/georust/geozero/pull/149 - once merged, i'll merge this one too. Thanks for all the hard work on this!

edpft commented 1 year ago

I created a https://github.com/georust/geozero/pull/149 - once merged, i'll merge this one too. Thanks for all the hard work on this!

Thanks for your speedy and comprehensive review, I'm just happy to help!

nyurik commented 1 year ago

I created a #149 - once merged, i'll merge this one too. Thanks for all the hard work on this!

Thanks for your speedy and comprehensive review, I'm just happy to help!

any time, glad you were able to contribute! Feel free to hack on more stuff :) For one, I see we do nasty and slow error formatting inside the impl_scalar_property_reader!, and I'm sure there are other places where we can do cleaner error handling (if that's something you want to do)

nyurik commented 1 year ago

P.S. @edpft tiny note: its not needed to name your branches <username>/something when doing a pull request from a fork - actually creates some confusion because your branch becomes username/username/something if considered together with the remote's name. Just name it something directly. One of those tiny nits :)

edpft commented 1 year ago

P.S. @edpft tiny note: its not needed to name your branches <username>/something when doing a pull request from a fork - actually creates some confusion because your branch becomes username/username/something if considered together with the remote's name. Just name it something directly. One of those tiny nits :)

Thanks for the tip, I wasn't aware of that!