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: handle plot file times under non-english locales #93

Merged

Conversation

altendky
Copy link
Collaborator

@altendky altendky commented Apr 7, 2021

Fixes #29

Draft for:

  • Self review
  • Merge of Drop Python 3.6 #94 to drop 3.6 support so we always have importlib.resources instead of having to use importlib_resources for now. Which isn't really a big deal... I dunno. Whatever.

@altendky altendky mentioned this pull request Apr 7, 2021
@altendky
Copy link
Collaborator Author

altendky commented Apr 8, 2021

I think my biggest doubt is about the goodness of bothering to test the whole log file loading right now including the funky work-around for the overly-ambitious .__init__(). Since I extracted the time parsing to a separate function, it may make sense to drop all the log file bits and just test that function directly instead. Then again, it might be nice to have the log file there for building future tests on.

@altendky altendky marked this pull request as ready for review April 8, 2021 03:31
@ericaltendorf
Copy link
Owner

I looked over this enough to mostly understand it, but I don't have enough experience to have an opinion if it's a great idea or not. Rubber-stamping approval.

@altendky
Copy link
Collaborator Author

altendky commented Apr 9, 2021

The faux-job test class is not good. Pendulum is well liked. Its time objects inherit from stdlib ones so they are interchangeable, one way anyways. We could have just temporarily set the locale but that's global process state and thus hazardous to threading. Maybe we could decide that isn't an issue here.

@altendky altendky merged commit 1fbbd65 into ericaltendorf:development Apr 9, 2021
@altendky altendky deleted the 29-locale_free_date_parsing branch April 9, 2021 14:40
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

Successfully merging this pull request may close these issues.

2 participants