Skip to content
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.

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

CYF London | Rihanna Poursoltani | Module-Tools | Sprint 2 | Shell Pipelines #36

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RihannaP
Copy link

@RihannaP RihannaP commented Mar 7, 2025

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

@RihannaP RihannaP added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Participant to add when requesting review labels Mar 7, 2025
Copy link

@ehwus ehwus left a comment

Choose a reason for hiding this comment

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

This is great work Rihanna, you've understood shell pipelines well and built on clearly very strong foundational shell tool knowledge. All of the scripts ran on my machine and the outputs were all exactly what was required. Keep it up!

@@ -4,3 +4,5 @@ set -euo pipefail

# TODO: Write a command to output the names of the files in the sample-files directory whose name contains at least one upper case letter.
# Your output should contain 11 files.
ls -1 ./sample-files | grep '[A-Z]'
ls -1 ./sample-files | grep '[A-Z]' | wc -l
Copy link

Choose a reason for hiding this comment

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

praise: Great to see that you've built in these 'tests' to your script, and good use of some of the other tooling that we've been learning to do it!

@@ -5,3 +5,4 @@ set -euo pipefail
# The input for this script is the scores-table.txt file.
# TODO: Write a command to output scores-table.txt, with shows the line for the player whose first score was the second highest.
# Your output should be: "Piotr Glasgow 15 2 25 11 8" (without quotes).
sort -k3 -rn ./scores-table.txt | tail -n+2 | head -n1
Copy link

Choose a reason for hiding this comment

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

question: This works well, but could we make this more efficient by using one program rather than both head and tail?

@@ -6,3 +6,5 @@ set -euo pipefail
# TODO: Write a command to show a list of all events that have happened, without duplication.
# The order they're displayed doesn't matter, but we never want to see the same event listed twice.
# Your output should contain 6 lines.
sort -u ./events.txt #with -u option
sort ./events.txt | uniq #with pipe output of sort to the input of uniq
Copy link

Choose a reason for hiding this comment

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

question: Which of these do you think is better? Are there any tradeoffs here?

While also great to see your working, it's probably best to comment out any duplication here as technically this script will output what we're expecting twice.

@@ -5,3 +5,5 @@ set -euo pipefail
# The input for this script is the events.txt file.
# TODO: Write a command to show how many times anyone has entered and exited.
# It should be clear from your script's output that there have been 5 Entry events and 4 Exit events.
sort ./events.txt | uniq -c #show Exit and Entry per person
Copy link

Choose a reason for hiding this comment

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

question: again it's good to see your working here, but let's comment out the first part as it doesn't exactly meet the specification

@@ -6,3 +6,4 @@ set -euo pipefail
# TODO: Write a command to show how many times anyone has entered and exited.
# It should be clear from your script's output that there have been 5 Entry events and 4 Exit events.
# The word "Event" should not appear in your script's output.
tail -n+2 ./events-with-timestamps.txt |sort -k3 | awk '{print $3}'| uniq -c
Copy link

Choose a reason for hiding this comment

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

nitpick: this is a great use of pipes and meets the criteria, but is the formatting and whitespace of this line consistent with the rest of your scripts?

@@ -6,3 +6,4 @@ set -euo pipefail
# The author got feedback that they're using too many exclamation marks (!).
#
# TODO: Write a command to output the contents of text.txt with every exclamation mark (!) replaced with a full-stop (.).
cat text.txt | tr '!' '.'
Copy link

Choose a reason for hiding this comment

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

question: this works well, but can you think of a way for tr to have access to the file directly so that you don't have to run two programs?

@ehwus ehwus added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review 📅 Sprint 2 Assigned during Sprint 2 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants