Allow passing pathlib.PosixPath to the function read_from_text_file()#787
Allow passing pathlib.PosixPath to the function read_from_text_file()#787xingularity wants to merge 4 commits into
pathlib.PosixPath to the function read_from_text_file()#787Conversation
Modified if statement to handle PosixPath instance file name in the same way with handling string file name.
Changes made in this commit: 1. Cross-platform type check with os.PathLike 2. Combine isinstance calls with isinstance(fname, (str, os.PathLike)) 3. Added tests for different types of path 4. Tighten error type by using FileNotFoundErrorn not Exceptions
xingularity
force-pushed
the
hotfix/handle-posix-path-issue-786
branch
from
a8ab00b to
9930fd0
Compare
|
Sorry maybe I didn't fully understand the test cases. I wonder are the test cases be able to catch the errors that happens in #786? |
Yes, test case |
yungyuc
changed the title
pathlib.PosixPath to the function read_from_text_file()
|
I changed the subject to be more informative. |
Added comment to `test_read_from_text_file_accepts_pathlib_path` to illustrate the background and purpose of this test case.
@yungyuc and @tigercosmos The comment has been added, you can review now. |
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
- Use inline annotation for discussions in the PR, not code comments.
- Also check for error messages, not just the exception type.
| finally: | ||
| os.unlink(path) | ||
|
|
||
| # Theis test is for a bug reported in issue #786: |
There was a problem hiding this comment.
It seems that you misunderstood what we meant by inline annotations. In #787 (review), "inline annotations in the PR" is like the inline comment I am leaving here. It is not code remarks/comments.
Please remove the unnecessary code comments and turn it into a PR inline annotation.
There was a problem hiding this comment.
Nitpick: there is a typo Theis.
| nd_arr = np.genfromtxt(io.StringIO(self.dlc_data), delimiter=',')[1:] | ||
| self.assertEqual(list(col_data), list(nd_arr[:, 1])) | ||
|
|
||
| def test_read_from_text_file_accepts_str_path(self): |
There was a problem hiding this comment.
This does not look like related to the fix. Please share with us why do you want to include it.
| def test_read_from_text_file_missing_raises_filenotfound(self): | ||
| tsdf = dataframe.DataFrame() | ||
| missing = pathlib.Path(tempfile.gettempdir()) / 'no_such_file.csv' | ||
| with self.assertRaises(FileNotFoundError): |
There was a problem hiding this comment.
Also check for error messages, not just the exception type.
3 participants
Modified if statement in modmesh/track/dataframe. This is to handle file name as a PosixPath instance.
This is for issue #786.