Skip to content

London | Elhadj Abdoul Diallo| Module-Tools | WEEK4 - Implement-cowsay - #39

Closed
eediallo wants to merge 9 commits into
CodeYourFuture:mainfrom
eediallo:implement-cowsay
Closed

London | Elhadj Abdoul Diallo| Module-Tools | WEEK4 - Implement-cowsay#39
eediallo wants to merge 9 commits into
CodeYourFuture:mainfrom
eediallo:implement-cowsay

Conversation

@eediallo

@eediallo eediallo commented Mar 8, 2025

Copy link
Copy Markdown

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 COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

@eediallo eediallo added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 8, 2025

@illicitonion illicitonion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good - I left a few questions to ponder :)

Comment thread implement-cowsay/cow.py Outdated
Comment thread implement-cowsay/cow.py Outdated
import argparse

parser = argparse.ArgumentParser(description="Make animals say things")
parser.add_argument('--animal', choices=cowsay.char_names, help="The animal to be saying things", default="cow")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not a problem, but is a bit unusual to have mixed quotes (some of the strings on this line are wrapped in 's and others "s) - is there a reason to prefer one over the other? Or a reason you used different ones here?

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.

No reason to be honest. Python accept both. I know I should pick one and be consistent with it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense :) Personally I prefer "s because it's more often that I write a string containing a ' than a ", e.g. I will often write a string literal like "I don't know" but I less often write a string literal like 'I said "hello"'

@illicitonion illicitonion 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 Mar 18, 2025
@illicitonion illicitonion added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 18, 2025
@illicitonion

Copy link
Copy Markdown
Member

Closing PR because the SDC run has finished. Feel free to re-open if you're still working on it.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants