Skip to content

Moving to cloudant library - #24

Closed
emord wants to merge 32 commits into
dimagi:jsonobjectfrom
emord:je/cloudant-lib
Closed

Moving to cloudant library#24
emord wants to merge 32 commits into
dimagi:jsonobjectfrom
emord:je/cloudant-lib

Conversation

@emord

@emord emord commented Oct 22, 2017

Copy link
Copy Markdown
Contributor

I started looking at this while I was in India, but never got around to submitting it.

The reason for doing this is that the cloudant library uses requests and supports python3. I've looked into moving restkit and couchdbkit to python3, but because it uses it's own socket library (socketpool), it's going to take a decent amount of work to get bytes and strings working correctly. Another plus is that anything we want in restkit/couchdbkit, we will have to do on our own vs cloudant has an obvious financial reason to keep their library up to date.

Just opening this up to see what you guys think of this approach. I'll also need to go back and check my work, but I think most of this should just work.

@dannyroberts ( i think you've done the most work on this lib)

@millerdev @nickpell (py3)

@mkangia (buddy)

Comment thread couchdbkit/client.py
else:
self.res = self.resource_class(uri, **client_opts)
self._uuids = deque()
self.cloudant_client = CouchDB('', '', url=uri, admin_party=True, connect=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why use admin_party=True? Does this change the auth mode of the database to admin party?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The url is passed in with the username/pass already in the uri. I think that was a temporary workaround since i'll need to change the way that's called

@nickpell

Copy link
Copy Markdown
Contributor

👏

Comment thread setup.cfg Outdated
with-coverage=1
cover-package=couchdbkit
# with-coverage=1
# cover-package=couchdbkit

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this just disabling coverage tracking? Any particular reason?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think that was just a local thing. I'll add it back before taking off open for review

Comment thread tests/client_test.py
db = self.Server.create_db('couchdbkit_test')
res = db.set_security({"meta": "test"})
self.assert_(res['ok'] == True)
del self.Server['couchdbkit_test']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any particular reason this test is being removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thikn this was because it doesn't work with couch2 #26

@emord emord mentioned this pull request Nov 10, 2017
Comment thread couchdbkit/client.py
raise
raise ResourceConflict

res = deepcopy(couch_doc)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be slow for large objects. Is there a more surgical method (copying less of the object) that could be used to avoid this?

@emord

emord commented Nov 17, 2017

Copy link
Copy Markdown
Contributor Author

The last commit doesn't actually work, and I feel a little unsure of the last 3 (all dealing with attachments).

Going to be breaking up this PR into smaller more easily understood ones

@emord emord mentioned this pull request Nov 17, 2017
@emord

emord commented Nov 20, 2017

Copy link
Copy Markdown
Contributor Author

closing this one in favor of #29

@emord emord closed this Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants