- Notifications
You must be signed in to change notification settings - Fork590
Add encoding option + fixed test that fails on windows.#238
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Zwork101 commentedOct 1, 2018
I'm going to work on this some more, I just need to use io.open instead. I'm not great at this cross-compatibility stuff. |
…s should be the last commit (please test gods).
msiemens left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the PR, I've got one change request, otherwise it looks good 🙂
Uh oh!
There was an error while loading.Please reload this page.
msiemens commentedOct 26, 2018
@Zwork101 Could you have a look at the code review? |
Zwork101 commentedOct 26, 2018
Oh, you're right sorry. Was going to deal with this a while ago and forgot. I'll write a note to do it later right now. |
msiemens commentedOct 30, 2018
No worries! Thanks for your contribution ❤️ |
msiemens commentedNov 6, 2018
This is now released in v3.12.0 🎉 |
To solve one of the issues presented in#222, the very simple option was allow the encodings kwarg to be passed in. I also added a test for it. After running the tests, I noticed
test_table_reprfailed. I believe this is because on linux based machines, it displays the memory location with lowercase letters, while windows uses uppercase and lowercase.