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

fix(learn): add test to return early pattern #38751

Merged
merged 3 commits into from May 7, 2020
Merged

Conversation

ERElli
Copy link
Contributor

@ERElli ERElli commented May 7, 2020

Currently no test for a = 0 or b = 0. This results in if(a<=0 || b<= 0) also being a valid solution

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

Closes #38750

Currently no test for a = 0 or b = 0. This results in if(a<=0 || b<= 0) also being a valid solution
@gitpod-io
Copy link

gitpod-io bot commented May 7, 2020

@raisedadead raisedadead added the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label May 7, 2020
@raisedadead
Copy link
Member

Hi @ERElli

Thanks for the quick PR.

We would need a third party confirmation on the issue thread (or anywhere else, like forum) before we can land this. Keeping this blocked for the moment.

@raisedadead raisedadead changed the title add: test to return early pattern fix(learn): add test to return early pattern May 7, 2020
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

Change the test case, to avoid this code from passing, as well:

if (a < 0 || b <= 0) {
  return undefined;
}

Co-authored-by: Shaun Hamilton <51722130+Sky020@users.noreply.github.com>
Copy link
Member

@ShaunSHamilton ShaunSHamilton left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@raisedadead raisedadead removed the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label May 7, 2020
Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM @ERElli 🎉 Congratulations on your first contribution to freeCodeCamp 🎉

For future reference, it's good to give your branch a more descriptive name and start it with a scope. See #3 just below here in the docs

@moT01 moT01 merged commit f757840 into freeCodeCamp:master May 7, 2020
@ERElli
Copy link
Contributor Author

ERElli commented May 7, 2020

Thanks @moT01!

I realized that I hadn't named my branch properly after I had made the PR and there were already comments. I wasn't sure if closing the PR and renaming the branch was the right thing to do.

I'll make sure to name my branch properly next time!

abbathaw pushed a commit to abbathaw/freeCodeCamp that referenced this pull request Jul 24, 2020
* add: test to return early pattern

Currently no test for a = 0 or b = 0. This results in if(a<=0 || b<= 0) also being a valid solution

* fix: typo in test text

* fix: update return early pattern test

Co-authored-by: Shaun Hamilton <51722130+Sky020@users.noreply.github.com>

Co-authored-by: Shaun Hamilton <51722130+Sky020@users.noreply.github.com>
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.

Missing test case in Basic JavaScript: Return Early Pattern for Functions
5 participants