Skip to content

London | 26-SDC-July | Kris Oldrini | Sprint 3 | Implement shell tools - #611

Open
XiaoQuark wants to merge 8 commits into
CodeYourFuture:mainfrom
XiaoQuark:implement-shell-tools
Open

London | 26-SDC-July | Kris Oldrini | Sprint 3 | Implement shell tools#611
XiaoQuark wants to merge 8 commits into
CodeYourFuture:mainfrom
XiaoQuark:implement-shell-tools

Conversation

@XiaoQuark

@XiaoQuark XiaoQuark commented Jul 27, 2026 •
edited by illicitonion
Loading

Copy link
Copy Markdown

Learners, PR Template

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

  • Completed cat, ls and wc implementation in JavaScript.

Task ID: CYF-1150

@XiaoQuark XiaoQuark added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 27, 2026
@XiaoQuark XiaoQuark added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 27, 2026
@LonMcGregor LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 4, 2026
LonMcGregor
LonMcGregor reviewed Aug 4, 2026
View reviewed changes

@LonMcGregor LonMcGregor left a comment

Copy link
Copy Markdown
Contributor

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, well organised code.I have some questions for you to push further

Comment thread implement-shell-tools/cat/cat.mjs Outdated
@@ -0,0 +1,53 @@
import process from "node:process";
import { promises as fs } from "node:fs";
import { Command } from "commander";

@LonMcGregor LonMcGregor Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a way you can get a program from commander directly?

Comment thread implement-shell-tools/ls/ls.mjs
import process from "node:process";
import fs, { truncate } from "node:fs";

const argv = process.argv.slice(2);

@LonMcGregor LonMcGregor Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason you don't use the commander or parseargs api here?

@LonMcGregor LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 4, 2026
@XiaoQuark

XiaoQuark commented Sep 11, 2026

Copy link
Copy Markdown
Author

Hi @LonMcGregor,
I have now amended the cat implementation and I'm importing program directly from commander.

Regarding the ls implementation, I wanted to experiment with handling flag parsing myself, so I intentionally chose not to use any libraries nor API.

@XiaoQuark XiaoQuark added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 11, 2026
@LonMcGregor

LonMcGregor commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

That makes sense, it's always good to practice. The task looks complete.

I would be interested if you could tell me some of the advantages or disadvantages you found when dealing with arguments in a nodejs app with and without a library?

@LonMcGregor LonMcGregor added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LonMcGregor LonMcGregor LonMcGregor left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@XiaoQuark @LonMcGregor