Conversation
Droid-An
added
the
Needs Review
|
Thanks for opening a new PR! AI started to review it |
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| def filter_hidden(files: list[str]) -> list[str]: | ||
| return [name for name in files if not name.startswith(".")] | ||
|
|
||
|
|
||
| def get_visible_entries(files: list[str], include_hidden: bool): | ||
| return files if include_hidden else filter_hidden(files) | ||
|
|
||
|
|
||
| def format_entries(files: list[str], one_per_line: bool): | ||
| if len(files) == 0: | ||
| return | ||
| print(("\n" if one_per_line else "\t").join(files)) |
There was a problem hiding this comment.
The parameter 'files' in 'format_entries', 'filter_hidden', and 'get_visible_entries' is used for both files and directory entries, which could be files or directories. Do you think a more general name like 'entries' would make it clearer that these lists can contain both files and directories?
| for directory, contents in result_dirs.items(): | ||
| print("\n" + directory + ":") | ||
| filtered = get_visible_entries(contents, include_hidden) | ||
| format_entries(filtered, one_per_line) |
There was a problem hiding this comment.
In the section where you print directory contents for multiple directories, the logic for filtering and formatting entries is repeated. If you needed to change how entries are displayed, you'd have to update it in more than one place. How could you refactor this to avoid repeating yourself?
This comment has been minimized.
This comment has been minimized.
Droid-An
added
Needs Review
|
Thanks for opening a new PR! AI started to review it |
| def get_visible_entries(files: list[str], include_hidden: bool): | ||
| return files if include_hidden else filter_hidden(files) |
There was a problem hiding this comment.
The function 'get_visible_entries' returns either all files or only non-hidden files, depending on the 'include_hidden' flag. The name 'get_visible_entries' might suggest it always returns only visible (non-hidden) entries, but it can also return hidden ones. Could you think of a name that more clearly describes what this function does based on its parameters?
Droid-An
added
Needs Review
|
Thanks for opening a new PR! AI started to review it |
| # Is a file? | ||
| if stat.S_ISREG(st.st_mode): | ||
| result_files.append(file_path) | ||
| # Is a directory? | ||
| if stat.S_ISDIR(st.st_mode): |
There was a problem hiding this comment.
I've noticed that you have comments like '# Is a file?' and '# Is a directory?' right above the code that checks if a path is a file or directory. Since the code itself (using stat.S_ISREG and stat.S_ISDIR) is already quite clear about what it's doing, these comments don't add much value. When code is self-explanatory, extra comments can sometimes make the code harder to read by adding clutter. How might you decide when a comment is truly helpful versus when the code speaks for itself?
2 participants
No description provided.