search

whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
162 stars 55 forks source link

Avoid Panic when extracting country id for UIFN numbers #54

Closed regismesquita closed 1 year ago

regismesquita commented 1 year ago

When trying to parse UIFN toll-free numbers, the lib panicked while retrieving the country id. This PR fixes this issue and returns None instead.

Co-authored-by: Raphael Costa vidal.raphael@gmail.com

rubdos commented 1 year ago

I wonder whether this should be a Result<Option<>> instead?

costaraphael commented 1 year ago

@rubdos yeah, using Result<Option<>> would mean no context for what failed would be lost (or maybe using just Result<> and making the None case one of the possible errors). The main issue is that it would be a breaking change for the API, so we decided to avoid it.

If you feel like introducing a breaking change is worth it in this case, then yeah, using a Result<> would have better semantics.

rubdos commented 1 year ago

@gferon opinion? Do we have more breaking changes scheduled? We could introduce it in two steps; merge as-is, and start a breaking version and break a lot of other things in the meantime if there's a demand.

rubdos commented 1 year ago

Fixing something like #55 would also introduce a breaking change. I'll make an issue that propagates this as an error, and start pinning such issues to a milestone for a new breaking release. @gferon, if you agree with the provided patch as non-breaking change, please do hit squash-and-merge!

rubdos commented 1 year ago

Looks like rustfmt disagrees here, would you mind still having a look, @regismesquita ?

regismesquita commented 1 year ago

@rubdos, sorry, I forgot to run it. I have pushed the formatted changes.

gferon commented 1 year ago

@gferon opinion? Do we have more breaking changes scheduled? We could introduce it in two steps; merge as-is, and start a breaking version and break a lot of other things in the meantime if there's a demand.

I think it's a good idea to merge this as-is and prepare a release for later. IIRC, fixing and merging https://github.com/whisperfish/rust-phonenumber/pull/27 will break some of the API (and maybe even is a good occasion to make some methods private).