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

Issue where key is found in word resolved. #4

Closed
wants to merge 3 commits into from

Conversation

MohammedSiddiqui
Copy link

  • Used regex and .replace function on string instead of .split and .join.
  • Created a basic test case to verify if this works.

MohammedSiddiqui10p and others added 3 commits June 3, 2016 19:46
two test cases are added to verify my work.
The Object test case was repeated twice, hence removed.
Basic ESLint fix.
used es6 template interpolation in expression
@umayr
Copy link
Owner

umayr commented Jun 7, 2016

I have couple of issues with this PR.

First thing is that you don't commit any experimental stuff in the repository. Commits [64c6ef3] and [3dc79d7] are not required for this change.

Secondly, unit tests. You don't add a describe block for one edge case. You add tests for any corner case in the already added blocks for the sake of consistency.

I'm closing this off in favor of #5. I have tweaked your changes to get the desired result. Moreover, I have added more unit tests.

Kudos for your work.

@umayr umayr closed this Jun 7, 2016
@MohammedSiddiqui
Copy link
Author

Understood, thank you for your feedback !

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.

3 participants