Skip to content

London | 25-SDC-July | Pezhman Azizi | Sprint 3 | Implement-shell-tools-with-NodeJS #169

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

London | 25-SDC-July | Pezhman Azizi | Sprint 3 | Implement-shell-tools-with-NodeJS #169

wants to merge 11 commits into from

Conversation

Pezhman-Azizi
Copy link

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

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

  • Start Sprint 3: Implement shell tools in Node.js
  • cat (Commit 1 – scaffold only):
    • Add cat/cat.js with CLI entry and basic arg parsing.
    • No functionality yet (prints usage when no args; placeholder message otherwise).
    • This prepares for next commits: basic output → -n → -b.

How to test (current scaffold)

# No args -> shows usage and exits non-zero
node cat/cat.js

# With any arg -> prints placeholder message
node cat/cat.js cat/sample-files/1.txt


## Questions

Is GNU-style error messages required later, or “close enough” is fine?

@github-actions GitHub Actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (sprint3) doesn't match expected format (example: 'Sprint 2', without quotes)

@Pezhman-Azizi Pezhman-Azizi changed the title London | 25-SDC-July | Pezhman Azizi |sprint3 | implement-shell-tools-with-NodeJS London | 25-SDC-July | Pezhman Azizi | Sprint3 | Implement-shell-tools-with-NodeJS Sep 21, 2025
@github-actions GitHub Actions
Copy link

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (Sprint3) doesn't match expected format (example: 'Sprint 2', without quotes)

@Pezhman-Azizi Pezhman-Azizi changed the title London | 25-SDC-July | Pezhman Azizi | Sprint3 | Implement-shell-tools-with-NodeJS London | 25-SDC-July | Pezhman Azizi | Sprint 3 | Implement-shell-tools-with-NodeJS Sep 21, 2025
@Pezhman-Azizi Pezhman-Azizi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 21, 2025
@LonMcGregor LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Sep 24, 2025
@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

LonMcGregor
LonMcGregor reviewed Sep 24, 2025
View reviewed changes
Copy link

@LonMcGregor LonMcGregor left a comment

Choose a reason for hiding this comment

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

Good start on this sprint's tasks, I have spotted a few areas where you could improve code further

implement-shell-tools/cat/cat.js
});
}

function numberFile(file, { nonblank }) {
Copy link

Choose a reason for hiding this comment

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

Is there a reason why you would want two separate functions for printing out content normally and numbered? Could these be combined into one function?

implement-shell-tools/ls/ls.js
const raw = process.argv.slice(2);

let includeAll = false; // -a
let onePerLine = false; // -1 (format already one-per-line)
Copy link

Choose a reason for hiding this comment

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

As it defaults to one per line, this argument is not doing anything. What change would you need to make to the program to ensure that the default behaviour is to output all on the same line?

implement-shell-tools/ls/ls.js
const targets = [];

// Parse flags (supports combined like -a1 / -1a). Treat lone "-" as a path.
for (const arg of raw) {
Copy link

Choose a reason for hiding this comment

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

Is there an advantage of writing argument parsing yourself rather than using an API or library?

@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 Sep 24, 2025
@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

2 similar comments
@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@LonMcGregor LonMcGregor LonMcGregor left review comments

Reviewers whose approvals may not affect merge requirements

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

Assignees
No one assigned
Labels
Reviewed Volunteer to add when completing a review with trainee action still to take.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Pezhman-Azizi @LonMcGregor