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

02_repeatString - Line-32, replace "odin" with "hey" #505

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bhagyeshsp
Copy link

The original code had "hey" as the seed string value and hence the comment on line-29 mentions "hey". But the last commit on the code changed "hey" to "odin".

We can either change instances of comment or the code. I propose changing the code instance from "odin" to "hey" for consistency.

Because

There is an inconsistency between the comment on line-29 and its corresponding code on line-32. It provides clarity to the learner by maintaining the same example of "hey" instead of "odin".

This PR

  • Replaced two instances of "odin" with "hey" on line-32

Issue

Closes #XXXXX

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. 01_helloWorld: Update test cases
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If this PR includes any changes that affect the solution of an exercise, I've also updated the solution in the /solutions folder

The original code had "hey" as the seed string value and hence the comment on line-29 mentions "hey". But the last commit on the code changed "hey" to "odin".

We can either change instances of comment or the code. I propose changing the code instance from "odin" to "hey" for consistency.
@MaoShizhong
Copy link
Contributor

In #460 the instructions were made clear that String.prototype.repeat should not be used and the implementation should be done manually via loops. I wonder if the team would agree that on top of fixing the 'hey' thing, the test be changed to

expect(repeatString('hey', number)).toBe('hey'.repeat(number));

Then the big regex explanation comment can be removed.

@bhagyeshsp
Copy link
Author

bhagyeshsp commented Nov 15, 2024

In #460 the instructions were made clear that String.prototype.repeat should not be used and the implementation should be done manually via loops.

Thanks, I saw #460 I'm unsure how it is related to this PR.
The current PR is more content-specific than code-specific.

I wonder if the team would agree that on top of fixing the 'hey' thing, the test be changed to

expect(repeatString('hey', number)).toBe('hey'.repeat(number));

EDIT: this PR does not propose this code change. This PR proposes retaining the same code currently present. odin replaced with hey in this code:

expect(repeatString('odin', number).match(/((odin))/g).length).toEqual(number);

This PR proposes getting the comment, code and example in alignment by maintaining "hey" throughout.

Then the big regex explanation comment can be removed.

I found the regex explanation educational.

@MaoShizhong
Copy link
Contributor

My comment was more an addition to yours and directed at the team when they review as opposed to directed at you, sorry if that wasn't clear. Regex was previously removed from comments and solutions in some of these exercises due to it being rather out of scope for this part of the curriculum. Hence the suggestion to the team that while we're at it, it may be best to apply the same approach here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants