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

Change dummy time.strptime call to use locale-independent format #2147 #2148

Merged
merged 2 commits into from May 16, 2018

Conversation

Projects
None yet
3 participants
@adam-iris
Contributor

adam-iris commented May 14, 2018

Fixes #2147

@megies megies added this to the 1.1.1 milestone May 15, 2018

@megies

megies approved these changes May 15, 2018

Looks good to me, thanks for letting us know about this one!

I don't think there's a chance we can add a test for this, as during our unittests obspy is already imported..

@megies

This comment has been minimized.

Member

megies commented May 15, 2018

@adam-iris, it'd be nice if you added a short changelog entry for this. Also, in the future I'd recommend working on dedicated branches for fixes/features, because now the maintenance_1.1.x branch on your fork is out of sync with the one on the main repo. You should avoid using those branches (master, maintenance..., ...) on your fork, in 99% of cases.

@krischer

This comment has been minimized.

Member

krischer commented May 15, 2018

I don't think there's a chance we can add a test for this, as during our unittests obspy is already imported..

We could set the locale to something not UTF8 for on test case runners. That might be ale to find some extra errors.

Otherwise I'm also fine with this PR. Thanks a bunch!

@megies

This comment has been minimized.

Member

megies commented May 15, 2018

We could set the locale to something not UTF8 for on test case runners. That might be ale to find some extra errors.

Good idea..

Travis has only UTF8 locales already set up seems like (https://github.com/travis-ci/travis-cookbooks/blob/4ef62935f9f21405bbc7b5e9023ff010410c172d/cookbooks/travis_build_environment/attributes/default.rb) so maybe we can start by at least using a chinese UTF8 locale for a start.. otherwise we'd need to generate the locale as well

@adam-iris

This comment has been minimized.

Contributor

adam-iris commented May 15, 2018

@megies sorry about the branch, I realized after submitting the PR that I hadn't followed the branching guidelines correctly.

I didn't test non-UTF8 locales, that does seem like a case that might still break. I agree that explicitly setting the locale (and then resetting it) around this call might be a safer option, but localization confuses and frightens me so I didn't dig too hard into this :P

@megies

This comment has been minimized.

Member

megies commented May 16, 2018

No worries @adam-iris . Just remember to either ignore your maintenance branch on the fork or resync it with the main repo one. Or well.. maintenance_1.1.x will not be used after the upcoming 1.1.1 release, so.. nevermind I guess.. ;-)

This had a pretty high likelyhood to hurt people so thanks again for letting us know. It's really good to have this fixed. non-UTF8 is rather esoteric though, I can't see how many people would use something like that..

@megies megies merged commit d0f6ad5 into obspy:maintenance_1.1.x May 16, 2018

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 28d0df6...e18e173
Details
codecov/project 88.07% (+1.13%) compared to 28d0df6
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docker-testbot Docker tests succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment