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

Allow on-disk files to be read using binary mode 'rb' #23

Closed
wants to merge 2 commits into from

Conversation

salcock
Copy link
Contributor

@salcock salcock commented Apr 20, 2021

The SimpleReader under python3 tends to get unhappy when asked to read a binary file from local disk.

The problem is that these files were always opened using 'r' mode, which means that python3 would treat the contents as utf-8 and try to decode whatever it reads into useful text. When the decoding inevitably fails, python3 would throw an exception.

This PR adds an optional mode argument to the SimpleReader init() method so that the reader can be forced to open the file in binary mode. wandio_open() has also been updated to allow rb as a valid mode, and will pass that on to the Reader init method.

Finally, I've also added a -m option to read_main() (aka pywandio-cat) which allows the mode to be specified via CLI argument.

salcock added 2 commits April 19, 2021 20:25
The option allows users to open files using 'rb' mode, so that
they can read binary files on local disk under Python3.

Updated read_main() to support setting the mode using -m
@salcock salcock requested a review from digizeph April 20, 2021 03:40
@salcock
Copy link
Contributor Author

salcock commented Apr 20, 2021

@digizeph Can you just double-check that I'm not breaking anything else here? -- r remains the default read mode, so in theory it should be OK but it never hurts to have another pair of eyes look over something like this.

@salcock
Copy link
Contributor Author

salcock commented Apr 21, 2021

Ok, I've just seen the other PR which more or less resolves the same problem.

I think we just need to settle on a solution and get one merged in, as this is starting to cause problems for other users. I think #21 is a better solution than this quick hack, so am happy to go forward with that (and will update my own pywandio using code as appropriate).

@digizeph
Copy link
Contributor

@salcock have you tried out the solution in #21 ? If that works for you then it might be a good choice to move forward. I will double check on my end to see if there is anything outstanding to leave that PR as draft.

@digizeph
Copy link
Contributor

digizeph commented Apr 21, 2021

Actually, I have a py3 branch that have fixes similar to what you have here. I think it's worthwhile to try that out.
1a9ba1d
It should handle both binary read and write well.

https://pypi.org/project/pywandio/

@digizeph
Copy link
Contributor

digizeph commented May 6, 2021

I have implemented the feature in #24 basic local tests work fine.
I also pushed a release of pywandio 0.3.0 to pypi at https://pypi.org/project/pywandio/0.3.0/

@digizeph digizeph closed this May 6, 2021
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