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

Fix error message for gettz when passed bytes #935

Merged
merged 4 commits into from Nov 2, 2019

Conversation

labrys
Copy link
Contributor

@labrys labrys commented Jul 8, 2019

Summary of changes

Fix error message for gettz when passed bytes

Closes #927

Pull Request Checklist

  • Changes have tests
  • Authors have been added to AUTHORS.md
  • News fragment added in changelog.d. See CONTRIBUTING.md for details

@labrys
Copy link
Contributor Author

labrys commented Jul 9, 2019

Results of timeit

Initial Test

python -m timeit -n 1000 -s "from dateutil.tz import gettz; gettz('America/New_York')"

# Before change
1000 loops, best of 5: 3.76 usec per loop
:0: UserWarning: The test results are likely unreliable. The worst time (241 usec) was more than four times slower than the best time (3.76 usec).

# After change
1000 loops, best of 5: 3.78 usec per loop
:0: UserWarning: The test results are likely unreliable. The worst time (204 usec) was more than four times slower than the best time (3.78 usec).

Optimized test

The initial test experiences a slow initial call (i believe during loading of tzfile) so I optimized it to only count subsequent gettz calls.

python -m timeit -n 1000 -s "from dateutil.tz import gettz; gettz('America/Los_Angeles')" "gettz('America/New_York')"

# Before change
1000 loops, best of 5: 2.42 usec per loop

# After change
1000 loops, best of 5: 2.28 usec per loop

Conclusion

The timeit differences are statistically insignificant

@pganssle pganssle changed the title Patch 1 Fix error message for gettz when passed bytes Aug 27, 2019
@pganssle pganssle added this to the 2.9.0 milestone Nov 2, 2019
@pganssle
Copy link
Member

pganssle commented Nov 2, 2019

@labrys Sorry for the delay in the review here, this mostly looks good, I've gone ahead and rebased this and pushed a few changes to it. We generally don't use xfail to enforce that a given exception is raised (instead use pytest.raises. Instead, we use xfail to write tests that should pass but don't because there's a bug (that way you get the regression test imposed immediately when you fix the bug).

I've gone ahead and refactored your tests. I also think that your example of object() being passed to gettz should probably also raise TypeError, since the AttributeError is exposing an arbitrary implementation detail, but I'll leave that for a different PR to fix.

I'm planning to cut a release tonight, so please let me know if you have any objections to my changes before then. I'll merge whenever you sign off or when it's closer to release time if I don't hear from you today.

@pganssle pganssle modified the milestones: 2.9.0, 2.8.1 Nov 2, 2019
@labrys
Copy link
Contributor Author

labrys commented Nov 2, 2019

@pganssle Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gettz on Python 3 fails with TypeError when passed bytes
2 participants