Skip to content

London | 25-SDC-July | Franklin Kamela | Sprint 4 | Feature/implement cowsay #159

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
Fradoka wants to merge 11 commits into
base: main
Choose a base branch
from
Open

London | 25-SDC-July | Franklin Kamela | Sprint 4 | Feature/implement cowsay #159

Fradoka wants to merge 11 commits into from
+25 −0

Conversation

@Fradoka
Copy link

@Fradoka Fradoka commented Aug 8, 2025 •
edited
Loading

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
Copy link

LonMcGregor commented Aug 19, 2025

Hi, I can't review your code yet. If you look at the "files changed" tab on this PR you will see the problem as there are hundreds of files that don't need to be there.

It looks like you're using a virtual environment, which is a good idea if you are using multiple libraries, but needs to be handled carefully within a VCS. Could you look up how to properly handle python virtual environments within git so this problem doesn't happen?

@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 Aug 19, 2025
@github-actions
Copy link

github-actions bot commented Aug 19, 2025

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
@github-actions
Copy link

github-actions bot commented Aug 19, 2025

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

@Fradoka Fradoka changed the title London | 25-SDC-July | Franklin Kamela | Sprint 4 | Module Tools | Feature/implement cowsay London | 25-SDC-July | Franklin Kamela | Sprint 4 | Feature/implement cowsay Sep 2, 2025
@Fradoka
Copy link
Author

Fradoka commented Sep 2, 2025

Hi @LonMcGregor, i have now deleted all unnecessary files. could you please 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
LonMcGregor reviewed Sep 2, 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 work implementing this, but I've left a comment in one area where it might be a bit over-engineered

implement-cowsay/cow.py
animal = args.animal.lower()

if not hasattr(cowsay, animal):
print(f"Invalid choice: '{args.animal}' (choose from: {', '.join(available_animals)})")
Copy link

@LonMcGregor LonMcGregor Sep 2, 2025

Choose a reason for hiding this comment

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

As you have specified the choices argument in, do you need to include this here as well? If you un with an invalid animal, you'll see some duplication of errors in the output

Copy link
Author

@Fradoka Fradoka Sep 2, 2025

Choose a reason for hiding this comment

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

Hi @LonMcGregor, i did see that but the example test case in README file % python3 cow.py --animal fish Turtles are cool too! showed the message as such. should i still edit this?

Copy link

@LonMcGregor LonMcGregor Sep 3, 2025

Choose a reason for hiding this comment

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

I forgot the readme explicitly requested that. In that case, this implementation is good and complete. Well done.

@LonMcGregor LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. 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 Sep 2, 2025
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. 📅 Sprint 4 Assigned during Sprint 4 of this module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fradoka @LonMcGregor