Skip to content

Birmingham | 2026-MAR-SDC | Joy Opachavalit | Sprint 4 | Implement shell tools in python#497

Open
enjoy15 wants to merge 9 commits into
CodeYourFuture:mainfrom
enjoy15:implement-shell-tools-in-python
Open

Birmingham | 2026-MAR-SDC | Joy Opachavalit | Sprint 4 | Implement shell tools in python#497
enjoy15 wants to merge 9 commits into
CodeYourFuture:mainfrom
enjoy15:implement-shell-tools-in-python

Conversation

@enjoy15

@enjoy15 enjoy15 commented Apr 15, 2026

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 all exercises

@enjoy15 enjoy15 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 4 Assigned during Sprint 4 of this module labels Apr 15, 2026

@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 start, but there is some work left to do here.

Is there a package you could use to help with managing arguments? Right now it is a bit complex, and you can't combine single-letter arguments like the command line tools.

Comment thread implement-shell-tools/ls/ls.py
@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 Jun 3, 2026
@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 4, 2026
@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 8, 2026
@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 8, 2026

@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 switching to using the argparse library. I do have some questions, as it looks like you are maybe not using the library as effectively as you could be.

Comment thread implement-shell-tools/ls/ls.py Outdated
Comment thread implement-shell-tools/ls/ls.py Outdated
@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 9, 2026
@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 9, 2026
@LonMcGregor

Copy link
Copy Markdown
Contributor

Thanks for replying @enjoy15 - Could you try implementing the ls using the standard argparse without subclassing or adding custom functions? It would help to reduce the amount of code that you have written, making it more readable and maintainable

@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 10, 2026
@enjoy15

enjoy15 commented Jun 11, 2026

Copy link
Copy Markdown
Author

@LonMcGregor Suggestion implemented. Thank you!

@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 11, 2026

@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.

Great, this looks a lot cleaner now. One question, see my comment

Comment thread implement-shell-tools/ls/ls.py Outdated
@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 16, 2026
@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 16, 2026

@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.

The -1 is no longer required, but I think you still need to think about how the -1 argument works, see my comment

Comment thread implement-shell-tools/ls/ls.py Outdated

locale.setlocale(locale.LC_COLLATE, "")
for entry in sorted(entries, key=locale.strxfrm):
print(entry)

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.

Think about the purpose of the -1 argument. How should the output differ when it is activated and when it is not? Does your implementation meet that expectation?

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.

Thank you for the catch! I realized that my original implementation was always printing one item per line, meaning the -1 flag wasn't actually changing the behavior.

I have updated the code so that it now checks for the one_per_line argument. If it's active, it prints one entry per line; otherwise, it outputs the entries horizontally, separated by spaces, to mimic the default ls behavior.

@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 17, 2026
@enjoy15 enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 17, 2026

@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.

Thanks, this ls implementation matches the spec now. well done

@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. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Jun 19, 2026
@enjoy15

enjoy15 commented Jun 19, 2026

Copy link
Copy Markdown
Author

Thanks, this ls implementation matches the spec now. well done

Thank you!

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