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

Add new accessor function getByKeys() #30

Merged
merged 3 commits into from
Sep 30, 2019
Merged

Add new accessor function getByKeys() #30

merged 3 commits into from
Sep 30, 2019

Conversation

spencerwi
Copy link
Contributor

@spencerwi spencerwi commented Sep 29, 2019

Added the getByKeys() method described in #28, and exercised it in some tests (and updated the README). Y'all's changelog appears autogenerated, so I didn't touch that, as a heads-up.

Sorry for the multiple "referenced a commit" over on the issue -- to keep things clean and down to a single commit, I kept force-pushing to my own fork repo (normally bad form, I know, but for a quick PR branch I figured it seemed alright).

Fixes #28

@codecov-io
Copy link

codecov-io commented Sep 29, 2019

Codecov Report

Merging #30 into master will increase coverage by 1.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #30     +/-   ##
=========================================
+ Coverage    69.1%   70.31%   +1.2%     
=========================================
  Files           5        5             
  Lines         123      128      +5     
  Branches       19       20      +1     
=========================================
+ Hits           85       90      +5     
  Misses         31       31             
  Partials        7        7
Impacted Files Coverage Δ
src/impl/domain.ts 73.91% <100%> (+0.77%) ⬆️
src/index.ts 61.53% <100%> (+2.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bde0c6...1658156. Read the comment docs.

@kabirbaidhya kabirbaidhya self-requested a review September 29, 2019 02:59
@kabirbaidhya kabirbaidhya changed the title Add getByKeys() method. Fixes #28 Add new accessor function getByKeys() Sep 29, 2019
@kabirbaidhya
Copy link
Contributor

@spencerwi Awesome. Thanks for contributing 🥇 👍 🙏.

There are few changes I'd request before signing off:

  1. Tweak the implementation for getByKeys - to return an identical "array" of values instead of an object. Please check the example I created in the issue New accessor function to retrieve a list of values by keys #28.

    This is the desired behavior:

    - const results = store.getByKeys(['a', 'b', 'sum']); // {a: 1, b: 2, sum: 3}
    + const results = store.getByKeys(['a', 'b', 'sum']); // [1, 2, 3]

    This gives you an ability to do this more intuitively:

    const [a, b, sum] = store.getByKeys(['a', 'b', 'sum']);
  2. Update tests, docblocks and README as per the above change.

@spencerwi
Copy link
Contributor Author

spencerwi commented Sep 29, 2019

Sure, I'll make those changes. I think I misunderstood the original issue.

In terms of destructuring, this is legal in Javascript:

// assuming the store has {a: 1, b: 2, sum: 3, empty: null}
let {a, b, sum, empty, notFoundThing} = store.getByKeys(['a', 'b', 'sum', 'empty', 'notFoundThing']);
console.log(a) // 1
console.log(b) // 2
console.log(sum) // 3
console.log(empty) // null
console.log(notFoundThing) // undefined

@spencerwi
Copy link
Contributor Author

I've made the changes you requested -- thanks for the feedback! Hopefully the PR in its current state better fits the desired use-case. 😄

Copy link
Contributor

@kabirbaidhya kabirbaidhya left a comment

Choose a reason for hiding this comment

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

Great. LGTM 🎉 💯

@kabirbaidhya kabirbaidhya merged commit d4a0cf7 into leapfrogtechnology:master Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New accessor function to retrieve a list of values by keys
3 participants