28-fix-pathlib-Path-glob#31
28-fix-pathlib-Path-glob#31sachindavra wants to merge 1 commit intodemocritus-project:mainfrom sachindavra:28-fix-pathlib-Path-glob
Conversation
fhightower
left a comment
fhightower
left a comment
There was a problem hiding this comment.
First, thank you for the pull request (PR) @sachindavra! The change you made is looking good. There are a few other things which need updated, though. I've made a couple of suggestions below. In addition to the changes I suggested, there are two other things we need to change before this PR is complete:
- On this line, we return
filesvariable.filesis a generator (because this is what the pathlib.Path.glob function returns). The problem is that this section of the function returns a list. When possible, it is a best practice to always return an item of the same type from a function, so we should update this section of the function to yield file names rather than append them to a list and return the list. - The tests for this PR are failing as you can see here. They are failing because the tests are expecting the function to return a list, but we are now returning a generator. We should update the tests to cast the response from the function to a tuple like:
assert tuple(python_file_names(...)) == ('a.py', 'a_test.py')`If you have any questions on either of these ^ points or if you would like me to work on them, let me know. Thanks again for the PR!
Fixes #28 .
| # from d8s_file_system import directory_file_names_matching | ||
| # | ||
| # files = directory_file_names_matching(path, '*.py') |
There was a problem hiding this comment.
We can remove these lines as they are no longer needed:
| # from d8s_file_system import directory_file_names_matching | |
| # | |
| # files = directory_file_names_matching(path, '*.py') |
| @@ -250,9 +250,12 @@ def python_copy_shallow(python_object: Any) -> Any: | |||
| # @decorators.map_first_arg | |||
| def python_file_names(path: str, *, exclude_tests: bool = False) -> List[str]: # noqa: CCR001 | |||
There was a problem hiding this comment.
The type hint for the return type of this function should be updated. Previously, this function returned a list. Now, as you can see here, it returns a generator (because this is what the pathlib.Path.glob function returns). Does that make sense?
| def python_file_names(path: str, *, exclude_tests: bool = False) -> List[str]: # noqa: CCR001 | |
| def python_file_names(path: str, *, exclude_tests: bool = False) -> Iterator[str]: # noqa: CCR001 |
|
Hi @sachindavra : Any progress on this issue? Thanks for creating the PR. I've added some feedback above which I'd like you to review and commit before we merge this. (This PR fixes #28 ) |
2 participants
No description provided.