Skip to content

Conversation

@asaniDev
Copy link

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

Changelist

Completed all the tasks in sprint 1

Questions

Ask any questions you have for your reviewer.

@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jun 10, 2025
@cjyuan cjyuan added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Jun 20, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Code is good and explanation is clear. Well done.

I only have a few suggestions and comments.

// 2. const penceStringWithoutTrailingP = penceString.substring(0, penceString.length - 1): removes the trailing 'p' from the string by using the substring method and passing it the start index (0) and the end index (length - 1) returning "399".
// 3. const paddedPenceNumberString = penceStringWithoutTrailingP.padStart(3, "0"): pads the string with leading zeros to ensure it has at least 3 characters, resulting in "399" remaining unchanged.
// 4. const pounds = paddedPenceNumberString.substring(0, paddedPenceNumberString.length - 2): extracts the pounds part of the string by taking all characters except the last two, resulting in "3".
// 5. const pence = paddedPenceNumberString.substring(paddedPenceNumberString.length - 2).padEnd(2, "0"): extracts the last two characters of the string and pads it with trailing zeros if necessary, resulting in "99" since it already has two characters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we expect this program to work as intended for any valid penceString if we deleted .padEnd(2, "0") from the code?
In other words, do we really need .padEnd(2, "0") in this script?

@cjyuan cjyuan 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. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jun 20, 2025
@asaniDev asaniDev added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 23, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

A commit with the message "updating to remote" have changed all the files in your branch (which also affected your Sprint-2 branches). You probably did not setup CRLF conversion properly.

You could fix the problem via

  1. Git commands (Ask ChatGPT what should i do if my previous push to GitHub did not convert the CR/LF properly?), or
  2. Create a new branch from your main branch, then transfer your files to the new branch, and commit the files in the proper folders.

If you choose option (2), you can also practice committing files one by one, on purpose, and for a reason. In VSCode, you can select which file to stage, and commit only the staged file. See: https://www.youtube.com/watch?v=z5jZ9lrSpqk&t=705 (At around 12:50 minute marker, the video shows how to stage a single file).


Please fix the branch. You should also do the same for your Sprint-2 branch.

@cjyuan
Copy link
Contributor

cjyuan commented Jul 23, 2025

  • The changes you made based on the comments were good.

  • Leaving responses directly in the comment threads makes tracking the discussion easier. You can try the approach in future PRs.

image

To find out what is the best practices, you can ask ChatGPT, how to respond to inline comments within a conversation thread in a pull request?

  • I will mark this PR as completed when you fixed this branch.

@cjyuan cjyuan added 👀 Review Git and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Jul 23, 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 with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants