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

RegExp pick #55

Merged
merged 8 commits into from
Feb 15, 2015
Merged

RegExp pick #55

merged 8 commits into from
Feb 15, 2015

Conversation

tristaaan
Copy link
Contributor

  • can do pick with RegExp and strings.
var sampleObject = {a: 1, B: 2, C: 3, d: 4}
pick(sampleObject, RegExp('[A-Z]')); // {B:2, C:3}
  • added -t 100 to package.json to ensure 100% test coverage

@tjmehta
Copy link
Owner

tjmehta commented Feb 10, 2015

Cool, I like this. Thanks @tristaaan will check this out tonight.

@tjmehta
Copy link
Owner

tjmehta commented Feb 10, 2015

Could you update the Readme as well?

@stoeffel
Copy link
Contributor

@tristaaan you should update the jsdoc in pick.js too.

@tristaaan
Copy link
Contributor Author

Updated readme and jsdoc. It was missing some minor test cases too. (was assuming an array's elements were all of the same type, either regexp or string)


function copyWithRegExp (from, to, regexp) {
return function (key) {
if (regexp.test(key) && !(key in to)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Pick should always override to (it should not work like 101/default)

@tjmehta
Copy link
Owner

tjmehta commented Feb 10, 2015

Thanks for the promptness. I am going to sleep on this one more night. I think I have some ideas on how to extend pick's functionality and I want to make sure what you've done here will play nice.

Idea: Support key-mapping functionality by supplying a hash of keys to function (similar to what is mentioned here: #49 (comment)).

@tjmehta
Copy link
Owner

tjmehta commented Feb 11, 2015

After thinking about this further I believe the RegExp functionality makes sense.
I like the cleaner implementation.

Just waiting on moving isRegExp to 101/is-regexp.

Everything else here LGTM. 👍

@tristaaan
Copy link
Contributor Author

isRegExp and tests were added by @stoeffel. Will be useful for match function #5.

@stoeffel
Copy link
Contributor

Just waiting on moving isRegExp to 101/is-regexp.

done

@tristaaan tristaaan mentioned this pull request Feb 15, 2015
@tjmehta
Copy link
Owner

tjmehta commented Feb 15, 2015

Thanks! 💯

tjmehta added a commit that referenced this pull request Feb 15, 2015
@tjmehta tjmehta merged commit f128ffa into tjmehta:master Feb 15, 2015
@tjmehta
Copy link
Owner

tjmehta commented Feb 15, 2015

Published in v0.11.0

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