loopbackio / loopback-connector-cloudant

LoopBack Connector for IBM Cloudant
Other
19 stars 21 forks source link

Cloudant Geospatial #191

Closed Danwakeem closed 6 years ago

Danwakeem commented 6 years ago

Description

Added a mixin for Cloudant Geospatial queries.

Related issues

Checklist

Danwakeem commented 6 years ago

I wasn't sure what I should do, but I think the errors in the unit tests are from downstream issues with loopback-connector-couchdb2. When I only run the tests for this repo and ignore the ones from loopback-connector-couchdb2 the tests all pass.

dhmlau commented 6 years ago

@Danwakeem , FYI - below is the error message of the failed test case:

  1) cloudant view.test - imported from couchdb2 couchdb2 view viewDocs returns result by quering a view:
      Uncaught AssertionError: expected 8 to be 4
      + expected - actual
      -8
      +4

      at Assertion.fail (node_modules/should/lib/assertion.js:180:17)
      at Assertion.prop.value (node_modules/should/lib/assertion.js:65:17)
      at node_modules/loopback-connector-couchdb2/test/view.test.js:56:37
      at Request._callback (node_modules/cloudant-nano/lib/nano.js:217:16)
      at Request.self.callback (node_modules/request/request.js:185:22)
Danwakeem commented 6 years ago

@dhmlau is there anything I need to do for that? I wasn't sure how it should be addressed since it is a test for another module.

dhmlau commented 6 years ago

@Danwakeem , thanks for incorporating my comments.
@jannyHou , could you please help? Thanks!

dhmlau commented 6 years ago

@Danwakeem , I'm not very familiar with this, so I've asked @jannyHou to take a look. Another thought.. Would it make sense to apply your PR on the couchdb connector loopback-connector-couchdb2 ?

Danwakeem commented 6 years ago

@dhmlau and @jannyHou I found the issue.

It looks like I was using the same key name in my test objects the viewDocs test was expecting to find in its own test, so I was unintentionally adding my data to that test.

I have added another commit to this PR to address this issue :)

Danwakeem commented 6 years ago

Well maybe there is another issue. It looks like the Node 6 build failed for some reason. I’ll keep looking!

dhmlau commented 6 years ago

You see the error when you test locally, right? anyway, the log on Travis didn't say what's wrong, so I kicked off the test again.

Danwakeem commented 6 years ago

No, that is what kind has me confused. When I run the tests locally they pass. The error on Travis was a socket hang up so I am not sure what is causing that yet.

Danwakeem commented 6 years ago

It looks like I have node 8 on my local machine though. I am going to pull down node 6 and re run locally.

Danwakeem commented 6 years ago

Hmm no dice on that one either. I pulled down node 6.14.2 and the tests still passed locally. I must have something locally that is different from travis.

Danwakeem commented 6 years ago

It is strange, when I ran the build for my fork in a new travis pipeline the tests passed. Do you think it could be something with Travis causing the build to fail?

virkt25 commented 6 years ago

Triggered travis again and it's all green (must've been something on their end).

@Danwakeem Can you please remove the package-lock.json file from this PR. We choose to not use them. If you like you can include a .npmrc file with package-lock=false to avoid it's generation all together.

@jannyHou Can you do a review? I looked over it and looks good (with the removal of package-lock.json) overall.

Danwakeem commented 6 years ago

@virkt25 Done! I went ahead and added it to the .npmrc file as well as the .gitignore file.

dhmlau commented 6 years ago

@slnode test please

Danwakeem commented 6 years ago

Hey @jannyHou, I have made the adjustments you had suggested above.

I also went ahead and cleaned up the comments for the geoDocs function. I used the view.js file as a starting place and didn't realize I left some things in there.

Let me know what you guys think about this round of changes. :)

dhmlau commented 6 years ago

I was surprised that CI all green previously. I just submitted a PR to update the node versions we use in Travis: https://github.com/strongloop/loopback-connector-cloudant/pull/192. Once that land, we can run the tests in this PR again.

Danwakeem commented 6 years ago

@dhmlau would you want me to pull in the update-travis branch or just wait for that PR to get merged into master?

dhmlau commented 6 years ago

@Danwakeem , the PR #192 just landed. Do you mind to rebase?

Danwakeem commented 6 years ago

Whoops, sorry about that I accidentally went too far back when I tried resetting my fork.

Danwakeem commented 6 years ago

I guess that is one way to get the rebase to work lol :)

Danwakeem commented 6 years ago

Yeah no problem thank you guys for helping me get that merged in! 😎

MarianZoll commented 6 years ago

@Danwakeem @jannyHou

Great to see this feature implemented with ds.connector in v2.1.0. Is there any effort done yet to add it to the README.md?

virkt25 commented 6 years ago

@MarianZoll Not that I am aware of. Would you like to open a PR with the changes to the README.md?

Danwakeem commented 6 years ago

@virkt25 @MarianZoll I went ahead and created another PR for the updates to the README. Since the API is so similar to views I just followed the same format for the view section.