Merge lp:~jameinel/u1db/doc-class into lp:u1db

Proposed by John A Meinel
Status: Merged
Merged at revision: 113
Proposed branch: lp:~jameinel/u1db/doc-class
Merge into: lp:u1db
Diff against target: 0 lines
To merge this branch: bzr merge lp:~jameinel/u1db/doc-class
Reviewer Review Type Date Requested Status
Samuele Pedroni Approve
Review via email: mp+82682@code.launchpad.net

Description of the change

This is an initial step to change the api over to using a Document class.

This changes the functions: get_doc, create_doc, put_doc.

The other apis aren't changed yet, though this by itself has already touched >2000 lines of code. Mostly in the test suite.

In the tests, I tried to switch from calling JSON strings "doc" to calling them "content". One of the big ones to change is tests.simple_doc and tests.nested_doc, since those are used by lots of tests. But I want that to be a separate patch.

The changes are roughly mechanical, so the big bits to discuss:

  1) I named the attribute "Document.doc_id", rather than "Document.id", as I don't really like using
     attribute names that mirror builtin functions.
  2) 'dirty' isn't currently used, and we could arguably get rid of set_content in favor of just
     setting content directly.
     I didn't really like having it be unclear that the current content in a Document doesn't
     actually match the revision listed.
     One option is to actually change the put_doc api. So rather than changing Document.content and
     calling put_doc(updated_doc), you could instead do:
       db.put_doc(doc, new_content)
     And then put_doc is responsible for updating the content and rev attributes.

To post a comment you must log in.
Revision history for this message
Samuele Pedroni (pedronis) wrote :

I would leave out the concept of dirty for now, if we need to move to a richer api to set/get from a document in general we can revisit this and would be more useful.

Just as a comment so it seems has_conflicts will need to be a three state value, not sure if it is worth or not but I suppose it's fine.

In _compare_and_insert_doc I suppose we should just not return a first value anymore,
the first return value is not used or tested anywhere as I can see. So this slipped through

154 - self._put_and_update_indexes(doc_id, cur_doc, doc_rev, doc)
155 + self._put_and_update_indexes(doc_id, cur_content, doc_rev, content)
156 return cur_doc, 'inserted'

this would have needed to be cur_content for consistency

157 - elif doc_rev == cur_rev:
158 + elif doc_rev == cur_doc.rev:
159 # magical convergence
160 - return cur_doc, 'converged'
161 + return cur_doc.content, 'converged'
162 elif cur_vcr.is_newer(doc_vcr):
163 # Don't add this to seen_ids, because we have something newer,
164 # so we should send it back, and we should not generate a
165 # conflict
166 - return cur_doc, 'superseded'
167 + return cur_doc.content, 'superseded'
168 else:
169 - return cur_doc, 'conflicted'
170 + return cur_doc.content, 'conflicted'

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/18/2011 4:19 PM, Samuele Pedroni wrote:
> Review: Approve
>
> I would leave out the concept of dirty for now, if we need to move
> to a richer api to set/get from a document in general we can
> revisit this and would be more useful.
>
> Just as a comment so it seems has_conflicts will need to be a three
> state value, not sure if it is worth or not but I suppose it's
> fine.

Not sure yet if we have to do that, or if
"get_docs(check_for_conflicts=False)" can just set it to False, and
not worry about it.

Note that the new api makes it a lot easier for applications to ignore
conflicts, since it is returned as an attribute of a struct, that they
have to take steps to pay attention to, rather than a part of the
return value. Though it always had to be a struct for C anyway.

>
> In _compare_and_insert_doc I suppose we should just not return a
> first value anymore, the first return value is not used or tested
> anywhere as I can see. So this slipped through
>
> 154 - self._put_and_update_indexes(doc_id, cur_doc, doc_rev, doc)
> 155 + self._put_and_update_indexes(doc_id, cur_content, doc_rev,
> content) 156 return cur_doc, 'inserted'
>
> this would have needed to be cur_content for consistency

Yeah, it probably evolved to no longer be used, because we just use
put_doc_if_newer() I think. So probably we just need to refactor.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk7GeIoACgkQJdeBCYSNAAPsWgCgx7bN8tM9VXKQGi1hJ/pvLwAK
HZkAoLCMLRmXpJOGeEKuctc4SRIhiNiH
=A9Tg
-----END PGP SIGNATURE-----

lp:~jameinel/u1db/doc-class updated
117. By John A Meinel

Get rid of set_content and dirty.

118. By John A Meinel

Minor fixup for _compare_and_insert.

119. By John A Meinel

TODO

Preview Diff

Empty

Subscribers

People subscribed via source and target branches