Skip to content

MRG, FIX: Fix encoding bug#7314

Merged
drammock merged 2 commits intomne-tools:masterfrom
larsoner:latin-1
Feb 13, 2020
Merged

MRG, FIX: Fix encoding bug#7314
drammock merged 2 commits intomne-tools:masterfrom
larsoner:latin-1

Conversation

@larsoner
Copy link
Copy Markdown
Member

Closes #7313

No need for latest.inc update because read_raw_nirx is a new function in 0.20

@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Feb 13, 2020

Why codecs.open and not just plain old open(..., encoding='latin1')?

@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Feb 13, 2020

https://docs.python.org/3.8/library/codecs.html says that open is recommended: "While the builtin open() and the associated io module are the recommended approach for working with encoded text files, this module provides additional utility functions and classes that allow the use of a wider range of codecs when working with binary files."

@larsoner
Copy link
Copy Markdown
Member Author

Just how I did it last time I had this problem, probably because of Python 2.7 or something. I'll switch to open

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 13, 2020

Codecov Report

Merging #7314 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7314      +/-   ##
==========================================
+ Coverage   89.83%   89.85%   +0.02%     
==========================================
  Files         449      449              
  Lines       80791    81481     +690     
  Branches    12880    13136     +256     
==========================================
+ Hits        72577    73216     +639     
- Misses       5387     5428      +41     
- Partials     2827     2837      +10     

@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Feb 13, 2020

TBH I wouldn't define a new function just for setting this one arg (mode='r' is the default).

@larsoner
Copy link
Copy Markdown
Member Author

TBH I wouldn't define a new function just for setting this one arg (mode='r' is the default).

There is some uncertainty about whether or not latin-1 is actually correct, so it is a bit of future proofing in the sense that if we need to use chardet or something similar to figure out the encoding we can do it in hopefully one place instead of many. But even without it this seems like a six of one half dozen of the other situation...

@drammock drammock merged commit 8de7309 into mne-tools:master Feb 13, 2020
@drammock
Copy link
Copy Markdown
Member

tx @larsoner

@larsoner larsoner deleted the latin-1 branch February 13, 2020 19:09
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* FIX: Fix encoding bug

* FIX: Use builtin
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* FIX: Fix encoding bug

* FIX: Use builtin
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.

Can't load raw nirx data, due to encoding issue

4 participants