Change Database.set() signature #34

Closed
mikek opened this Issue Nov 25, 2011 · 8 comments

Comments

Projects
None yet
3 participants

mikek commented Nov 25, 2011

set([doc_id], data, callback[, attachments=None]) has a really strange signature. It is the only method in trombi, which has first optional positional argument. Arguments parsing is manual (which can be then simplified). The real reason to change it, however, is inability to pass callback keyword argument. This does not allow to use tornado.gen to simplify asynchronous code.
I guess it can't be handled without changing method's signature and breaking existing code. At least the first argument would become required: set(doc_id, data, callback[, attachments=None]).
Please, let me know if there is a clean way to get around this particular issue.

Member

akheron commented Nov 25, 2011

The first argument is intentionally optional. If it wasn't optional and you already had a Document instance, you'd have to say db.set(doc.id, doc, callback) instead of just db.set(doc, callback). Making the callback keyword arg is possible without making doc_id a required argument.

mikek commented Nov 25, 2011

Yeah, an ugly workaround is to respect additional callback keyword argument and cowardly append it to the *args. Requires nothing but trivial changes at the top of set() method:

     def set(self, *args, **kwargs):
+        cb = kwargs.pop('callback', None)
+        if cb:
+            args = list(args)
+            args.append(cb)
+            args = tuple(args)
         if len(args) == 2:
             data, callback = args
             doc_id = None
         elif len(args) == 3:
             doc_id, data, callback = args
         else:
-            raise TypeError(
-                'Database.set expected 2 or 3 arguments, got %d' % len(args))
+            raise TypeError('Database.set is missing callback argument')

This kind of change looks not so terrible looking at how it is now. Cleaner but backwards-incompatible way would require to change order of arguments.

Member

akheron commented Nov 25, 2011

Here's a more compact version:

     def set(self, *args, **kwargs):
+        cb = kwargs.pop('callback', None)
+        if cb:
+            args += (cb,)
         if len(args) == 2:
             data, callback = args
             doc_id = None
         elif len(args) == 3:
             doc_id, data, callback = args
         else:
-            raise TypeError(
-                'Database.set expected 2 or 3 arguments, got %d' % len(args))
+            raise TypeError('Database.set is missing callback argument')

I think this is the way to go.

@nailor Comments?

Member

nailor commented Nov 25, 2011

@mikek Thanks for the bug report! Making everything work with tornado.gen is top priority before 1.0 release of trombi and it's more than desired that bugs like this surface!

@akheron Your patch seems fine, go ahead and commit :)

Member

akheron commented Nov 27, 2011

The proposed error message is bad. If called with e.g. set(a, b, c, callback=d), the error message is 'Database.set is missing callback argument'.

Member

nailor commented Nov 28, 2011

Yeah, that's true. Maybe something along the lines TypeError("Database.set takes at most 2 non-keyword arguments.")

This is a bit misleading, as the user can give: set(a,b,c), which essentially has 3 positional arguments. However, I'm planning to update the documentation in a way that set will have clearly documented callback-keyword. Thus enabling the use of set(a, b, c) should be considered as a backwards compatible way of doing things.

Member

akheron commented Nov 28, 2011

Sounds good to me.

Member

nailor commented Nov 28, 2011

Hmm. Github failed to close this, however this is now fixed in b0a45d2

@nailor nailor closed this Nov 28, 2011

truemped pushed a commit to truemped/trombi that referenced this issue Jan 15, 2012

Database: Accept callback as keyword argument in Database.set
To support tornado.gen mechanism, we'll now on accept callback as a
keyword argument for Database.set.

Fixes GH-34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment