-
-
Notifications
You must be signed in to change notification settings - Fork 42
Manchester | 25-SDC-Nov | Geraldine Edwards | Sprint 3 | Implement Shell Tools #242
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?
Manchester | 25-SDC-Nov | Geraldine Edwards | Sprint 3 | Implement Shell Tools #242
Conversation
4c263a8 to
f82bbb0
Compare
…roved readability
f82bbb0 to
13fab23
Compare
Add package.json and package-lock.json for dependency management
LonMcGregor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on these tasks, sorry for the late review
Good use of comments to explain your code
I've left comments in some areas where you could improve your code
implement-shell-tools/ls/ls.js
Outdated
| const sortedEntries = sortEntries(filteredEntries); | ||
|
|
||
| // print entries for -1 flag (one per line) | ||
| printEntries(sortedEntries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does your code differ if the user does not give the -1flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for this @LonMcGregor! I hadn't finished formatting the output for not using the -1 flag, oops! Fixed it now :)
| } | ||
| if (files.length > 1) { | ||
| // print the totals as wc does | ||
| printWcOutput(totalLines, totalWords, totalBytes, "total", options, noFlags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen if I give only a single option, e.g. node wc.js sample-files/1.txt -c? Does this print correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @LonMcGregor :) I have spotted my error in the program options configuration and changed the remainder of the file as needed to the long flag names.
…ns names in printWcOutput function accordingly
|
Good work, this looks finished now |
Learners, PR Template
Self checklist
Changelist
This PR contains the javascript exercises in the cat, ls and wc directories of the implement-shell-tools directory
Questions
I'd be interested to see what ways I could refactor the code in the files and also match the exact wc output in my file, thanks.