Skip to content

Conversation

Fradoka
Copy link

@Fradoka Fradoka commented Aug 8, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Questions

@Fradoka Fradoka added 📅 Sprint 4 Assigned during Sprint 4 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 8, 2025
@LonMcGregor LonMcGregor added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 19, 2025
Copy link

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

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

Reason: Wrong number of parts separated by |s

1 similar comment
Copy link

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

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

Reason: Wrong number of parts separated by |s

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.

Remember to update the default PR text when submitting on github.

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

Choose a reason for hiding this comment

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

Have you tested that the numbering is working correctly when multiple files are passed in?

Choose a reason for hiding this comment

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

Have you tested to make sure your -1 argument is working correctly?

Choose a reason for hiding this comment

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

There is some duplication in this code, can you have a look at reducing it?

Also, when you are outputting, make sure that the indentation is working right.

@Fradoka Fradoka changed the title London | 25-SDC-July | Franklin Kamela | Module Tools | Sprint 4 | Feature/shell-tools-python London | 25-SDC-July | Franklin Kamela | Sprint 4 | Feature/shell-tools-python Aug 31, 2025
Copy link

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

@Fradoka
Copy link
Author

Fradoka commented Sep 1, 2025

Hi @LonMcGregor, i have now addressed the reviews.

@Fradoka Fradoka added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Sep 1, 2025
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 work reducing the duplication in wc, and while the column-printing solution in ls might be a bit more complicated than is necessary for this task, that's good practice, well done.

In cat I have the same comment as I made in your js implementation - is it necessary to count line numbers and non blank line numbers separately?

@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. labels Sep 2, 2025
@Fradoka
Copy link
Author

Fradoka commented Sep 2, 2025

hi @LonMcGregor, i dont actually have to use 2 separate counters. i have now amended the code code to let -b overide -n like in real cat and used only 1 counter to address review.

@Fradoka Fradoka 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 2, 2025
@LonMcGregor
Copy link

Great work

@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 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed. 📅 Sprint 4 Assigned during Sprint 4 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants