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

Time zone capital or lowercase #96

Closed
scotthavens opened this issue Aug 12, 2019 · 8 comments
Closed

Time zone capital or lowercase #96

scotthavens opened this issue Aug 12, 2019 · 8 comments

Comments

@scotthavens
Copy link
Contributor

The time_zone value in the config file has to specifically be from the pytz list with correct captial case. For example, mst does not work but MST does. However, utc works when it should be UTC so somewhere SMRF is modifying the value.

https://stackoverflow.com/questions/13866926/is-there-a-list-of-pytz-timezones

@scotthavens
Copy link
Contributor Author

While PR #145 address a lot of the timezone handling, inicheck is parsing the time_zone to lower. The only way around at the moment is to have inicheck load the config file, change the time_zone then pass the UserConfig object to run_smrf.

The solution to this would be to have inicheck type to not change the string at all.

@scotthavens
Copy link
Contributor Author

There is a RawString field in katana that solved this problem.

https://github.com/USDA-ARS-NWRC/katana/blob/6a408cbf0b88aadf98a4ef8c58bd50901936f0ce/katana/CoreConfig.ini#L137

@jomey
Copy link
Collaborator

jomey commented Jun 11, 2020

Alternatively you could use a little filter to match the input string against the available from ptyz

Example for 'mst' as given string, will return 'MST'

import re

pattern = re.compile('^mst$', re.I)
next(filter(pattern.match, pytz.all_timezones))

@scotthavens
Copy link
Contributor Author

Would this make users lazy in not looking up the actual timezone name? Just passing the string through is working now with minimal changes. The gotcha is that pytz has both a utc and UTC which is why it's worked so far. However, if the tests are changed to UTC all the gold files will have to be changed as well just for the time zone attribute...

scotthavens added a commit to scotthavens/smrf that referenced this issue Jun 11, 2020
scotthavens added a commit to scotthavens/smrf that referenced this issue Jun 11, 2020
@jomey
Copy link
Collaborator

jomey commented Jun 11, 2020

I am mainly suggesting a case-insensitive lookup of the user input. Has the additional advantage that it is checked against valid ones and can catch errors early

@scotthavens
Copy link
Contributor Author

Fully understand. We need to figure out what is better, case insensitive or not?

@jomey
Copy link
Collaborator

jomey commented Jun 11, 2020

You make a good point regards enforcing/educating users to lookup proper timezone names.
The doc can always refer to the accepted list: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

@jomey
Copy link
Collaborator

jomey commented Jun 11, 2020

For down the road: USDA-ARS-NWRC/inicheck/issues/53

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

No branches or pull requests

2 participants