Conversation
illicitonion
added
NotCoursework
Needs Review
|
Thanks for opening a new PR! AI started to review it |
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
|
|
||
| try: | ||
| for path in args.paths: | ||
| line_num = 1 |
There was a problem hiding this comment.
The variable 'line_num' is initialized inside the for loop for each file. This means the line numbering restarts for each file, which is fine if that's the intended behavior. If you wanted to number lines across all files, where would you move the declaration of 'line_num'?
There was a problem hiding this comment.
This seems like an overly "what if" question for the AI reviewer, and may confuse the trainee into thinking something is wrong?
| return parser.parse_args() | ||
|
|
||
|
|
||
| def filter_hidden(files: list[str]) -> list[str]: |
There was a problem hiding this comment.
The variable 'files' is used in several functions, but sometimes it refers to file names and sometimes to directory entries. Could you think of a name that would make it clearer what kind of data is being stored in each case?
There was a problem hiding this comment.
I don't think this comment is accurate? I only see files used used to refer to file names?
There was a problem hiding this comment.
It seems the ai assumes that when a user runs ls sample/file.txt, the main function passes ['sample/file.txt'] as files to the helper functions. From ai's perspective this value is a directory entry, rather than a file name.
| return [name for name in files if not name.startswith(".")] | ||
|
|
||
|
|
||
| def get_visible_entries(files: list[str], include_hidden: bool): |
There was a problem hiding this comment.
The variable 'files' is used in several functions, but sometimes it refers to file names and sometimes to directory entries. Could you think of a name that would make it clearer what kind of data is being stored in each case?
| return files if include_hidden else filter_hidden(files) | ||
|
|
||
|
|
||
| def format_entries(files: list[str], one_per_line: bool): |
There was a problem hiding this comment.
The variable 'files' is used in several functions, but sometimes it refers to file names and sometimes to directory entries. Could you think of a name that would make it clearer what kind of data is being stored in each case?
There was a problem hiding this comment.
As above (but also, ideally it would only have left this comment once?)
| 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 logic for filtering visible entries (with or without hidden files) is used in several places. How might you refactor your code so that this logic is only written once and reused wherever needed?
There was a problem hiding this comment.
This looks pretty extracted to me already - do you know what this is trying to point at?
There was a problem hiding this comment.
probably it doesn't like these two identical lines of code
filtered = get_visible_entries(contents, include_hidden)
| if stat.S_ISDIR(st.st_mode): | ||
| result_dirs[file_path] = os.listdir(file_path) | ||
|
|
||
| result_files = get_visible_entries(result_files, include_hidden) |
There was a problem hiding this comment.
The logic for filtering visible entries (with or without hidden files) is used in several places. How might you refactor your code so that this logic is only written once and reused wherever needed?
| if len(file_paths) == 1: | ||
| entries = list(result_files) | ||
| for contents in result_dirs.values(): | ||
| filtered = get_visible_entries(contents, include_hidden) |
There was a problem hiding this comment.
The logic for filtering visible entries (with or without hidden files) is used in several places. How might you refactor your code so that this logic is only written once and reused wherever needed?
|
|
||
| for directory, contents in result_dirs.items(): | ||
| print("\n" + directory + ":") | ||
| filtered = get_visible_entries(contents, include_hidden) |
There was a problem hiding this comment.
The logic for filtering visible entries (with or without hidden files) is used in several places. How might you refactor your code so that this logic is only written once and reused wherever needed?
|
|
||
| for file_path in file_paths: | ||
| stats = os.stat(file_path) | ||
| count = {"lines": 0, "words": 0, "bytes": stats.st_size} |
There was a problem hiding this comment.
The variable 'count' is used to store line, word, and byte counts for a file. Would a more descriptive name, like 'file_counts' or 'stats', make it clearer what this variable holds?
There was a problem hiding this comment.
This seems like a reasonable, but slightly pedantic, comment
3 participants
No description provided.