-
Notifications
You must be signed in to change notification settings - Fork 25
count implementation #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I'll be reviewing this PR |
|
I'll be reviewing this |
|
This command works exactly as expected. I would suggest adding more detail to your count.md file. Currently, for your command options, it just says count or count ..., but you could change it to something like "count [word] [directory]" so it's clearer to understand how to run the code. Additionally, there are some indentation errors in your code. Mostly in count and count_in_file. Otherwise, you did a great job and fulfilled everything you set out to do! |
|
I'll be code reviewing this |
|
Overall the code looks good and I can see your test cases in your readme. All the main stuff is present but I am confused about what the KMP algo is as there was no explanation for that. I do like how if I enter an input that isn't valid there is some error handling. I would maybe just want the code to output only the files where there is a match and not all the files in total. Other than that I like this High Level Checks:
Code Checks:
|
|
Cool idea, and the implementation looks good! Thanks for already addressing some of the previous CR comments! I am a bit iffy on the listed of visited inodes... I'd exclude '.' and '..' instead because now we can only fit 22 entries in the list and that seems too small for even a toy OS like we are using. Maybe you were building that because of worries about links? I can't find the justification, maybe I missed it. |

General Overview: count command is similar to grep, as it looks through files for a specific word, and lists the number of times the word appeared within each file, and provides a total count at the end.