search

uwmadison-chm / bioread

Utilities to work with files from BIOPAC's AcqKnowlege software
MIT License
66 stars 23 forks source link

Support for alternative encoding #26

Closed louis-youpling closed 4 years ago

louis-youpling commented 4 years ago

This commit allows support for multiple encoding against UTF-8 only before. It should solve uwmadison-chm/bioread/#24

njvack commented 4 years ago

Hi!

Thank you so much for the PR!

Would you mind separating the functional changes from the stylistic ones here? I don't want to reformat the project's code at this point, and the formatting changes make it hard to see what's going on.

Also, do you have an example file this allows reading? Adding that (feel free to modify one of the ones in test/data if you can) and a test would be helpful.

Also also, is this fix for python 2, python 3, or both?

louis-youpling commented 4 years ago

I have removed the formatting changes. Will add a sample file after removing PHI and associated test. This is for python 3.6.7 (Anaconda).

njvack commented 4 years ago

Hm... I don't think you need to upload a test file, actually — what this PR probably actually does is switch bioread to a python 3 program?

Are there really AcqKnowledge files with encodings other than UTF-8?

louis-youpling commented 4 years ago

See a90f14fa559a17fe392e20d82ceb125df75f5fab.

njvack commented 4 years ago

One last thing! Would you bump the version to 1.1.0 with this? I haven't had time to test this locally yet but it's definitely a breaking change.