Repository navigation
Implement basic functionality for cat command with line numbering - #19
matthewjustice wants to merge 1 commit into
Conversation
matthewjustice
added
NotCoursework
Needs Review
![cyf-ai-code-reviewer[bot]](https://avatars.githubusercontent.com/u/22743767?s=60&v=4){kind=small}
| for filename in args.files: | ||
| line_number = 1 | ||
| try: | ||
| with Path(filename).open(encoding="utf-8", newline="") as source: | ||
| for line in source: | ||
| number_line = args.n or (args.b and bool(line.strip())) | ||
| if number_line: | ||
| sys.stdout.write(f"{line_number:6}\t") | ||
| line_number += 1 |
There was a problem hiding this comment.
In your current implementation, line_number is defined and initialized inside the for filename in args.files: loop. That means line_number is reset to 1 for each file, so numbering starts over for every new file in a multi-file invocation.
On both common cat implementations (GNU on Linux and BSD on macOS), when you use -n or -b with multiple files, the line numbers continue increasing across files rather than restarting at 1 for each file. Since your README says this tool should act the same as cat when run from this directory, resetting line_number per file will give different output from the real cat in multi-file cases.
Would you consider where line_number needs to live so that it can persist across the whole run instead of being recreated for each file? Moving it to a scope that spans all files would let numbering continue correctly across files while still letting you increment it inside the inner loop.
To "like" or "dislike" this comment, please follow this link
| @@ -0,0 +1,34 @@ | |||
| """Concatenate files, optionally numbering their lines.""" | |||
There was a problem hiding this comment.
The module docstring on line 1 explains what the script does at a high level. In many small scripts, the combination of the filename (cat.py), the function/argument names, and the argument help texts already make the intent very clear. In those cases, an extra docstring can end up repeating information without adding much new context.
It might be worth asking yourself: Does this comment tell a future reader anything they couldn’t already infer from the code and CLI help? If the answer is no, you could either drop the docstring or expand it to include non-obvious behavior (for example, how -n and -b interact, or how errors are handled). That way, any comment you keep is clearly pulling its weight.
To "like" or "dislike" this comment, please follow this link
cyf-ai-code-reviewer
Bot
added
Reviewed
and removed
Needs Review
|
The CYF AI review has left comments. It will only review a PR one time. When you have addressed these comments, please request review again and a volunteer will take a look. |
This PR is for testing the cyf-ai-code-reviewer