Skip to content

Allows dot-notation to match against a complex structure when using matchesKeyInQuery #4308

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

Closed
wants to merge 2 commits into from

Conversation

bohemima
Copy link
Contributor

As the title suggests, this will allow dot notation to be used for matchesKeyInQuery, our use case is that we have a pointer we want to match against, simply using result[key] does not cut it.

Unfortunately I have not enough experience or time at the moment to write proper tests for this, so anyone is welcome to help with this PR.

@codecov
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

Merging #4308 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4308      +/-   ##
==========================================
- Coverage    92.5%   92.48%   -0.03%     
==========================================
  Files         118      118              
  Lines        8246     8246              
==========================================
- Hits         7628     7626       -2     
- Misses        618      620       +2
Impacted Files Coverage Δ
src/RestQuery.js 95.47% <100%> (ø) ⬆️
src/RestWrite.js 93.21% <0%> (-0.19%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.5% <0%> (-0.12%) ⬇️

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 84aadba...038e759. Read the comment docs.

@montymxb
Copy link
Contributor

@bohemima unfortunately without any proper testing we can't really start to go through the process to review, even if it is just the one line.

To help you out you would put a test down here in ParseQuery.spec.js . A format for such a test could be something like:

it('should match complex structure with dot notation', function(done) {
 // test code in here
// call done() to succeed
// call done.fail() to fail
});

You can find plenty of examples of how to write a test case up by looking at any of the existing tests. If you take the time to put together something, even if it's not quite right, we'll work with you on it. That's what open source is about 👍 .

@montymxb montymxb requested review from montymxb and removed request for montymxb November 1, 2017 20:18
@montymxb montymxb self-assigned this Nov 1, 2017
@montymxb
Copy link
Contributor

montymxb commented Nov 1, 2017

@bohemima thanks! I've assigned myself to this, I'll take a look at it later 👍 .

@@ -335,7 +335,7 @@ RestQuery.prototype.replaceNotInQuery = function() {
const transformSelect = (selectObject, key ,objects) => {
var values = [];
for (var result of objects) {
values.push(result[key]);
values.push(key.split('.').reduce((o,i)=>o[i], result));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change this for transformSelect we should also change the similar code in transformDontSelect. Additionally another test should then be added to account for doesNotMatchKeyInQuery having this modified behavior as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, totally missed that. I'll give it a go soon (tm)

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Overall a nice little addition here. The extra change would have to be made over here on line 393 (as mentioned in the code), but that's just another one liner like what you've done already.

@flovilmart Although the tests indicate this is ok (and it does seem to be working just fine with the change) is there anything you might think of that this could cause an issue with?

@flovilmart
Copy link
Contributor

@bohemima ping?

@acinader
Copy link
Contributor

fixed by: #4399

@acinader acinader closed this Nov 30, 2017
@montymxb
Copy link
Contributor

montymxb commented Dec 2, 2017

Thanks again @bohemima for getting this started, and thanks @acinader for finishing it!

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.

4 participants