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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse and build timezone data into stubs #134

Merged
merged 8 commits into from
Mar 7, 2024

Conversation

percivalalb
Copy link
Contributor

@percivalalb percivalalb commented Oct 26, 2023

See #132

@coveralls
Copy link

coveralls commented Oct 26, 2023

Coverage Status

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

@DrHyde
Copy link
Owner

DrHyde commented Dec 2, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

DrHyde commented Dec 5, 2023

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 :-)

build-data.stubs Show resolved Hide resolved
lib/Number/Phone.pm Outdated Show resolved Hide resolved
lib/Number/Phone/StubCountry.pm Show resolved Hide resolved
t/timezones.t Show resolved Hide resolved
@DrHyde
Copy link
Owner

DrHyde commented Mar 5, 2024

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
Copy link
Contributor Author

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 DrHyde marked this pull request as ready for review March 7, 2024 21:26
@DrHyde DrHyde merged commit 3940331 into DrHyde:master Mar 7, 2024
5 of 22 checks passed
@DrHyde
Copy link
Owner

DrHyde commented Mar 7, 2024

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

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.

3 participants