Skip to content

NW | 2026-mar-sdc | Zabihollah Namazi | Sprint 3 | implement shell tools#442

Open
ZabihollahNamazi wants to merge 5 commits into
CodeYourFuture:mainfrom
ZabihollahNamazi:ZabihollahNamazi-implement-shell-tools
Open

NW | 2026-mar-sdc | Zabihollah Namazi | Sprint 3 | implement shell tools#442
ZabihollahNamazi wants to merge 5 commits into
CodeYourFuture:mainfrom
ZabihollahNamazi:ZabihollahNamazi-implement-shell-tools

Conversation

@ZabihollahNamazi
Copy link
Copy Markdown

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

hi
could you please check the answers? thanks

@ZabihollahNamazi ZabihollahNamazi added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 27, 2026
@SlideGauge SlideGauge added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label May 30, 2026
Copy link
Copy Markdown

@SlideGauge SlideGauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several notes from me, could you address them please?

Comment thread implement-shell-tools/cat/cat.js Outdated
}
} else if (showAllNumbers) {
// -n: number all lines
console.log(`${lineNumber} ${line}`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it reproduce the same formatting the real cat does?

Comment thread implement-shell-tools/cat/cat.js Outdated
lines.forEach((line) => {
if (showNonEmptyNumbers) {
// -b: number only non-empty lines
if (line.trim() !== "") {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

real cat considers a line with whitespaces to be non-empty line

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it with if (line !== "")

for (const path of paths) {
try {
// read file as text
const content = await fs.readFile(path, "utf-8");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the whole algorithm place a trailing new line by the end of the file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the implementation prints using console.log, so it always appends a newline after output

console.log(element)
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does real ls do if no command-line arguments are passed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve updated the implementation so the default behaviour now properly handles this and filters hidden files unless -a is passed

Comment thread implement-shell-tools/ls/ls.js Outdated
const direc = await fs.readdir(path)
// if the path is <sample-files> console.log(direc) gives us =>: [ '.hidden.txt', '1.txt', '2.txt', '3.txt', 'dir' ]

if(showOnePerLine && showAllFilesWithHidden){
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if I pass only -a?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Ive now separated -a and -1 so they work independently like real ls. -a controls visibility of hidden files, and -1 controls output format

Comment thread implement-shell-tools/wc/wc.js Outdated
const paths = args.filter(arg => !arg.startsWith("-")) || ".";
console.log(paths)

// const direct = await fs.readdir(path);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code - should be removed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it , Thanks

Comment thread implement-shell-tools/wc/wc.js Outdated
let totalWords = 0;
let totalchars = 0;

if(showLines){
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this else if logic support multiflag execution?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you’re right — I changed it to support multiple flags by using separate if conditions instead of else if

Comment thread implement-shell-tools/wc/wc.js Outdated

if(showLines){
for (const path of paths){
const content = await fs.readFile(path, "utf-8");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if file is absent?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the code , now , If a file is missing, program prints an error and keeps running for the remaining files

Comment thread implement-shell-tools/wc/wc.js Outdated
totalWords += words;
totalchars += char;
}
console.log("lines: ", totalLines, " words", totalWords, " char:", totalchars)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation should be adjusted

Comment thread implement-shell-tools/wc/wc.js Outdated
totalWords += words;
totalchars += char;
}
console.log("lines: ", totalLines, " words", totalWords, " char:", totalchars)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real wc does not write "lines: ", "wods" and so on, instead, it right-aligns numbers and uses tab

@SlideGauge SlideGauge added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels May 30, 2026
@ZabihollahNamazi ZabihollahNamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants