🐛 fix loading CSV ‘books’ if the path contains ‘.’ #47
Conversation
The CSV book reader erronously split the path by _all_ dots, instead of just the last one. This is what os.path.splitext() is for, but since it drops the leading dot, I kept the split() call to keep the change minimal.
Codecov Report
@@ Coverage Diff @@
## master #47 +/- ##
=======================================
Coverage 97.08% 97.08%
=======================================
Files 42 42
Lines 2951 2951
=======================================
Hits 2865 2865
Misses 86 86
Continue to review full report at Codecov.
|
|
OK. I will have a look. Here is the overall architecture: http://pyexcel.readthedocs.io/en/latest/architecture.html. You are welcome to ask more questions. |
| @@ -305,7 +305,7 @@ def _load_from_file(self): | |||
| self.__line_terminator = self._keywords.get( | |||
| constants.KEYWORD_LINE_TERMINATOR, | |||
| self.__line_terminator) | |||
| names = self._file_name.split('.') | |||
There was a problem hiding this comment.
what if we do this: os.path.abspath(self._file_name), which then expands both '.' and '..' into absolute path? And maybe we could do os.path.splitext()..?
There was a problem hiding this comment.
As far as I can tell, neither pyexcel nor pyexcel-io currently refer to os.path.abspath(), and using it in this case seems like overkill. In my opinion, using os.path.splitext() is most correct, although it requires changing two lines below. Would you rather I submit that change?
There was a problem hiding this comment.
let's choose the most correct change - os.path.splitext(). I will write a few test cases later.
The 0.5.7 release of pyexcel-io fixes loading CSV files from directories containing '.' in their name, which was the motivation for our fork. See also <pyexcel/pyexcel-io#47>.
The 0.5.7 release of pyexcel-io fixes loading CSV files from directories containing '.' in their name, which was the motivation for our fork. See also <pyexcel/pyexcel-io#47>.
The CSV book reader erronously split the path by all dots, instead
of just the last one. This is what os.path.splitext() is for, but
since it drops the leading dot, I kept the split() call to keep the
change minimal.
I tried to create a testcase for this, but it turned out to be harder than expected since I don't quite understand the internal design of pyexcel.