-
Notifications
You must be signed in to change notification settings - Fork 2
Add timezone package #11
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
Conversation
jonathonmcmurray
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a tzinfo file in TorQ (I think it's just a q flat file) https://github.com/DataIntellectTech/TorQ/blob/master/config/tzinfo (based off Kx supplied data at https://github.com/KxSystems/cookbook/tree/master/timezones)
Would it make sense to include that instead of the python script etc.? I guess we might have to update it periodically if countries change observance of DST etc. but that's probably pretty minimal
|
Updated to take from flat and provided steps for manual update. Have also aligned code with style.md taking this to be the golden source. Will also update other PRs to align as I work on them |
jonathonmcmurray
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple of minor points & something I need to update in style guide
|
Updated to reflect updated style guide for timezone ending brace, fixed unit test comments and added additional notes in doc that tzinfo is available by default and download steps are only needed for updating |
Tested :
Notes:
Unsure best way to have this data sourced. Python to me is the most lightweight and easily automatable but does not really fit the objective of a standalone q package. That being said one way or another this package will be reliant on external data. This to me seemed good to include, although maybe for actually shipping of this package its included more as an aside.
Other options for this would be building out the HTTP q library and see if I can replicate this logic in q alone. Will take a look at that