Skip to content

Location Timezone support for python3 newstr #244

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

Closed
lukewitmer opened this issue Oct 4, 2016 · 3 comments
Closed

Location Timezone support for python3 newstr #244

lukewitmer opened this issue Oct 4, 2016 · 3 comments
Labels
Milestone

Comments

@lukewitmer
Copy link

Things have been quiet for a while. Hope the fall is going well for all of you so far!

The following code

from __future__ import (absolute_import, division,
                        print_function, unicode_literals)
from builtins import *
from pvlib.location import Location
tz = -10
str_tz = 'Etc/GMT+' + str(-1*int(tz))
my_loc = Location(20.0, -155.9, str_tz, 105.0, "my_place")

gives the following exception:

Traceback (most recent call last):
  File "/Users/lucas/Documents/Code/sim/pv.py", line 52, in <module>
    my_loc = Location(20.0, -155.9, str_tz, 105.0, "my_place")
  File "/usr/local/lib/python2.7/site-packages/pvlib/location.py", line 76, in __init__
    raise TypeError('Invalid tz specification')
TypeError: Invalid tz specification

The most straightforward fix is, in location.py, to check isinstance(tz, basestring) instead of str on line 66. This makes it compatible across python 2 and 3.

@lukewitmer
Copy link
Author

The fix is just one word in location.py, but I can't make the change as I seemingly don't have permission to push my branch for a PR. I'm fine if someone else simply makes the fix, but if I can have permission to contribute to branches for PRs, that would be nice :) thanks!

git -c diff.mnemonicprefix=false -c core.quotepath=false -c credential.helper=sourcetree push -v --tags --set-upstream origin refs/heads/Issue244-location-compatibility:refs/heads/Issue244-location-compatibility 
Pushing to https://github.com/pvlib/pvlib-python.git
remote: Permission to pvlib/pvlib-python.git denied to lukewitmer.
fatal: unable to access 'https://github.com/pvlib/pvlib-python.git/': The requested URL returned error: 403
Completed with errors, see above

@wholmgren
Copy link
Member

To maybe make things a little more clear for future readers, the problem is that in the example above, str_tz = 'Etc/GMT+' + str(-1*int(tz)) is not actually a pure str. Rather, the from __future__ import unicode_literals statement means that Python 2 will interpret that "string" as unicode (same behavior as the default in Python 3). In Python 2, this presents a problem for the Location class constructor since it's looking for str but getting unicode instead.

We'd welcome a fix for this. To make a PR, you need to fork the library, push the changes to your fork, and make the PR from your fork. Please also add a test to test_location.py, probably just by adding u'Etc/GMT+7' or similar to the test_location_tz parametrize list.

@lukewitmer
Copy link
Author

Thanks Will. I wasn't familiar with the forking workflow. I think I have it now. PR is in and was running through some checks. Thanks!

@wholmgren wholmgren added the bug label Oct 4, 2016
@wholmgren wholmgren modified the milestones: 0.4.1, 0.4.2 Oct 4, 2016
@wholmgren wholmgren modified the milestones: 0.4.3, 0.4.2 Dec 7, 2016
@wholmgren wholmgren modified the milestones: 0.4.4, 0.4.3 Dec 28, 2016
@wholmgren wholmgren modified the milestones: 0.4.5, 0.4.4 Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants