Skip to content

Conversation

@sigod
Copy link

@sigod sigod commented Apr 7, 2020

Implementing it with moment-timezone would've been prettier, but this way is simpler, more flexible and adds no extra dependencies.

@b3by
Copy link
Owner

b3by commented Apr 18, 2020

Thank you for your PR. This all comes new to me, I haven't though about a feature like a time offset yet. What would be the benefit of introducing a time offset option compared with the timezone? I like the clean code you submitted, and the tests, and everything else, I'm just not 100% sure this feature belongs here and not in a different time management package.

@sigod
Copy link
Author

sigod commented Apr 18, 2020

Honestly, for my use case timezone selection would've been enough (development VM with incorrect timezone and changing PC's timezone is highly undesired at this time). But when looking at how it can be implemented I figured offset in minutes is way more flexible and requires less maintenance in the long run. We don't need to worry about timezones of some countries changing in the future or about daylight saving time or about keeping timezone libraries up to date.

Ultimately, the decision is up to you.

I like the clean code you submitted, and the tests, and everything else

Thank you. Only I forgot to update documentation. Give me a heads up if you'll want to merge this PR, so I can update the documentation before the merge.

@b3by
Copy link
Owner

b3by commented Apr 18, 2020

Sure thing, thanks a lot for that. I will need some time to dive again into the project, as it's been a while since the last time I worked on it, but I remember I had something in place for timezones using moment-timezone that was not a complete nightmare to maintain and scale. If I realize the effort is not worth, then I'll definitely integrate this!

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.

2 participants