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

feat(contains): create exercise #442

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

Conversation

NikitaRevenco
Copy link
Contributor

@NikitaRevenco NikitaRevenco commented Mar 23, 2024

Because

It was decided to add new recursion exercises

Previous

This PR

  • Adds exercise 14

Issue

Related to #27265

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

@NikitaRevenco NikitaRevenco marked this pull request as draft March 23, 2024 14:51
@NikitaRevenco NikitaRevenco changed the title New exercise | Contains (14) New exercise | Contains Mar 23, 2024
@NikitaRevenco NikitaRevenco marked this pull request as ready for review March 23, 2024 19:23
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

I imagine you're leaving out contains.spec.js to make it easier to do changes to only one file instead of two, but just a reminder in case that wasn't your intention and you just forgot!

14_contains/solution/contains-solution.js Outdated Show resolved Hide resolved
14_contains/solution/contains-solution.spec.js Outdated Show resolved Hide resolved
14_contains/solution/contains-solution.spec.js Outdated Show resolved Hide resolved
14_contains/solution/contains-solution.spec.js Outdated Show resolved Hide resolved
@MaoShizhong
Copy link
Contributor

@NikitaRevenco to make things easier to know when to do things, once you've made any requested changes etc., if you could re-request a review from whichever maintainer, that would make it much easier to know when it's okay to review (so we don't go to review and submit comments right before you push another commit that changes things again)

@NikitaRevenco
Copy link
Contributor Author

@NikitaRevenco to make things easier to know when to do things, once you've made any requested changes etc., if you could re-request a review from whichever maintainer, that would make it much easier to know when it's okay to review (so we don't go to review and submit comments right before you push another commit that changes things again)

Alright, that makes sense!

@NikitaRevenco
Copy link
Contributor Author

@NikitaRevenco to make things easier to know when to do things, once you've made any requested changes etc., if you could re-request a review from whichever maintainer, that would make it much easier to know when it's okay to review (so we don't go to review and submit comments right before you push another commit that changes things again)

I've opened the next exercise for review! I wanted to 'request' - but seems like that cannot be done on a PR with no reviews yet. For the future, do you prefer me to add a comment when the next PR is ready for review or?

@MaoShizhong
Copy link
Contributor

MaoShizhong commented Mar 24, 2024

@NikitaRevenco No need to comment or request a review when un-drafting something. The un-drafting itself is enough to signify that something is ready for review.

I'm not reviewing these because I'm assigned them, nor am I the only one allowed to review them. I just happen to be seeing these and choosing to add my reviews. Any of the maintainers may wish to review (or not) at any time, and it wouldn't be unrealistic if they had different opinions to me or comments on things I hadn't considered. And I'd probably be wanting at least another opinion before they're all finalised for merging.

Copy link
Contributor

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

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

A few more comments.

14_contains/solution/contains-solution.js Outdated Show resolved Hide resolved
14_contains/solution/contains-solution.js Outdated Show resolved Hide resolved
@NikitaRevenco NikitaRevenco changed the title (14) New exercise | Contains feat(contains): create exercise Aug 9, 2024
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

@JoshDevHub Any further thoughts yourself?

14_contains/README.md Show resolved Hide resolved
14_contains/solution/contains-solution.spec.js Outdated Show resolved Hide resolved
14_contains/contains.spec.js Outdated Show resolved Hide resolved
Copy link
Contributor

@JoshDevHub JoshDevHub left a comment

Choose a reason for hiding this comment

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

Looks good I think 👍

14_contains/solution/contains-solution.spec.js Outdated Show resolved Hide resolved
14_contains/contains.spec.js Outdated Show resolved Hide resolved
@MaoShizhong MaoShizhong added the Status: Do Not Merge This PR should not be merged until further notice label Aug 20, 2024
14_contains/README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Do Not Merge This PR should not be merged until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants