LONDON | 26-SDC-JUly | Boshra Mahmoudi | Sprint 4 | Implement shell tools in python - #678
Conversation
|
|
||
| for filename in args.files: | ||
|
|
||
| with open(filename) as file: |
There was a problem hiding this comment.
What happens if filename doesn't exist? You might try with error handling for FileNotFoundError.
There was a problem hiding this comment.
@Khantdotcom I added try and except so it can show an error message when the file does not exist.
| for filename in args.files: | ||
|
|
||
| with open(filename, "rb") as file: | ||
| content = file.read() |
There was a problem hiding this comment.
Reading the whole file with .read() is putting that data into RAM. What if you point this at a 50GB log file? Check out this code for learning a better approach here
There was a problem hiding this comment.
@Khantdotcom I didn’t have any clue about this. Thanks, I’ll use this approach next time.
There was a problem hiding this comment.
@BoshraM It's completely okay with your current approach! The whole idea of suggested one is just chunking, which means reading the file in part by part. Hope you learned something.
| print(file, end=" ") | ||
| print() | ||
|
|
||
| else: |
There was a problem hiding this comment.
what happens if we try like python ls.py completely_fake_file.txt?
Your current condition checks if it's a directory but what if it is a file. Shall we just print or do something with that condition?
And what if it's neither of dir nor file? How do we handle that?
There was a problem hiding this comment.
@Khantdotcom thanks, I didn’t pay attention to that issue. I added another check so it now handles existing files, directories, and nonexistent paths.
|
@Khantdotcom I have switched labels from Needs Review to Reviewed. I think you have forgotten to do this :) |
|
@abdishakoor-dev thanks for noticing that. I forgot to ask admin about that label settings. Somehow I cannot edit labels in this Module-Tools. What do you suggest me to do? |
|
@Khantdotcom do you have access to the Slack code review channel? Can you post on their please so this issue is picked up by the team. |
Thanks, appreciate your help. |
|
Marked the label as "Reviewed" and "Complete" |
Task code
CYF-1152