search

DrHyde / perl-modules-Number-Phone

Number::Phone and friends
24 stars 31 forks source link

Parse and build timezone data into stubs #134

Closed percivalalb closed 6 months ago

percivalalb commented 11 months ago

See #132

coveralls commented 11 months ago

Coverage Status

coverage: 99.526% (+0.002%) from 99.524% when pulling 72c3934ce548fcad1a3deb0a7f2a2feda5eddf66 on percivalalb:alb/timezones into e16ab6b32b2b6b4b6eb32284786fd32f43916e97 on DrHyde:master.

DrHyde commented 9 months ago

I need to go through it in more detail, but first impressions are that it looks good. I'm mostly just commenting to let you know that I'm not ignoring this :-)

percivalalb commented 9 months ago

Hey, thanks for the heads up. If just implementing for stub-countries then it is probably near completion with a few tweaks needed here or there, I haven't gone down the rabbit hole of trying to return a single timezone for non-stub countries.

I'd have to confirm but i think using just Number::Phone you might get a stub country or not so timezones() might return undef for some but not most, which might make it a little confusing if you didn't realise you needed to use ::Lib to always get non-undef.

I'd also want to check what happens in the case of international numbers like +800 and what we should do there.

DrHyde commented 9 months ago

I think for all the weird international ranges like +800 the only reasonable responses are either [] or [every possible timezone]. Thankfully Inmarsat no longer has different prefixes for each ocean :-)

DrHyde commented 6 months ago

Is there anything else left that you plan to do on this? I think it looks like it's ready for release, although I think that at first I'll mark it as being experimental and subject to minor change.

percivalalb commented 6 months ago

It should be good to go.

At the start we talked about making a function that returned a single "best" timezone, though this would need some work to determine what is "best". I'm not planning to work on that at the moment, but could be a future addition.

DrHyde commented 6 months ago

This will be in the next release, which will be some time this weekend