After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 640812 - Support default values for parameters
Support default values for parameters
Status: RESOLVED OBSOLETE
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Low enhancement
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
: 652136 (view as bug list)
Depends on: 558620
Blocks: 693405
 
 
Reported: 2011-01-28 10:56 UTC by Guillaume Desmottes
Modified: 2018-01-10 20:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test app (292 bytes, text/x-python)
2012-04-24 07:22 UTC, Guillaume Desmottes
  Details
tests: Add tests for multiple utf8 args with allow-none (1.88 KB, patch)
2013-07-28 01:08 UTC, Simon Feltman
committed Details | Review
Cleanup invoke args and kwargs combiner code (5.34 KB, patch)
2013-07-28 04:18 UTC, Simon Feltman
reviewed Details | Review
Add support for default arguments annotated with allow-none (11.12 KB, patch)
2013-07-28 04:30 UTC, Simon Feltman
none Details | Review
Add support for default arguments annotated with allow-none (11.11 KB, patch)
2013-07-28 04:54 UTC, Simon Feltman
needs-work Details | Review
Remove overrides which supply callback user_data as a keyword arg (1.17 KB, patch)
2013-07-28 05:48 UTC, Simon Feltman
reviewed Details | Review
docs: Add a keyword value of None for allow-none annotations (4.04 KB, patch)
2013-07-28 07:23 UTC, Simon Feltman
committed Details | Review
Move PyGI direction code into new function (3.44 KB, patch)
2013-07-31 05:58 UTC, Simon Feltman
none Details | Review
Replace callable arg cache array+len with GPtrArray (16.93 KB, patch)
2013-07-31 05:58 UTC, Simon Feltman
none Details | Review
Remove special case marshaling for instance arguments (13.40 KB, patch)
2013-07-31 05:58 UTC, Simon Feltman
none Details | Review
gimarshalingtests: Add tests for mixed allow-none and implicit args (3.62 KB, patch)
2013-08-06 08:29 UTC, Simon Feltman
committed Details | Review
cache refactoring: Add comments to callable cache structure (1.34 KB, patch)
2013-08-06 08:33 UTC, Simon Feltman
committed Details | Review
cache refactoring: Move PyGI direction code into new function (3.46 KB, patch)
2013-08-06 08:33 UTC, Simon Feltman
committed Details | Review
cache refactoring: Replace callable arg cache array+len with GPtrArray (17.18 KB, patch)
2013-08-06 08:33 UTC, Simon Feltman
committed Details | Review
cache refactoring: Remove special case marshaling for instance arguments (13.42 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
cache refactoring: Use bit field for PyGIDirection instead of enum (11.81 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
cache refactoring: Remove continue statements from _args_cache_generate (6.82 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
cache refactoring: Move variable declarations to blocks where they are used (3.08 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
cache refactoring: Move argument count increments outside of main logic (4.20 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
reviewed Details | Review
cache refactoring: Cleanup array length argument marshaling (10.41 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
cache refactoring: Move arg cache field assignments into _arg_cache_new (2.71 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
Add support for default arguments annotated with allow-none (15.79 KB, patch)
2013-08-06 08:34 UTC, Simon Feltman
committed Details | Review
Don't pass None to callbacks when user data is not specified (4.74 KB, patch)
2013-09-27 02:56 UTC, Simon Feltman
committed Details | Review
Refactor GLib.child_watch_add to make it more testable (7.14 KB, patch)
2013-09-28 09:35 UTC, Simon Feltman
committed Details | Review
Refactor GLib.io_add_watch to make it more testable (2.99 KB, patch)
2013-09-28 09:35 UTC, Simon Feltman
committed Details | Review
Add support for variable user data arguments (22.96 KB, patch)
2013-09-28 09:35 UTC, Simon Feltman
committed Details | Review

Description Guillaume Desmottes 2011-01-28 10:56:01 UTC
When you have an _async method using the GIO async pattern, you can't pass None as callback, even if you annotate it with allow-none.

** (process:25416): CRITICAL **: g_arg_info_is_caller_allocates: assertion `info != NULL' failed

** (process:25416): CRITICAL **: g_arg_info_is_caller_allocates: assertion `info != NULL' failed

** (process:25416): CRITICAL **: g_arg_info_is_caller_allocates: assertion `info != NULL' failed
Traceback (most recent call last):
  • File "./text-handler.py", line 44 in handle_channels_cb
    display_pending_messages(channel)
  • File "./text-handler.py", line 32 in display_pending_messages
    channel.ack_messages_async(messages, None, None)
  • File "/usr/lib/pymodules/python2.6/gtk-2.0/gi/types.py", line 40 in function
    return info.invoke(*args)
TypeError: Error invoking TelepathyGLib.ack_messages_async: Invalid callback given for argument callback

Comment 1 Martin Pitt 2012-04-23 17:11:53 UTC
Thanks for you report. Is this still an issue with PyGObject 3.2? If so, can you please attach or point to a piece of code which reproduces this?
Comment 2 Guillaume Desmottes 2012-04-24 07:22:37 UTC
Created attachment 212666 [details]
test app

    manager.prepare_async(None)
doesn't work but
    #manager.prepare_async(None, None, None)

The callback is allow-none (bug #653484) but the user_data is not. Shouldn't we always annotate user_data of GAsyncCallback with (allow-none) as well?
Comment 3 Martin Pitt 2012-04-25 07:32:26 UTC
(In reply to comment #2)
> Created an attachment (id=212666) [details]
> test app
> 
>     manager.prepare_async(None)
> doesn't work but

Right, you'll get 

TypeError: prepare_async() takes exactly 4 arguments (2 given)


>     #manager.prepare_async(None, None, None)

This does seem to work for me, the program is running without an error message. Can you please clarify what's broken here?

> The callback is allow-none (bug #653484) but the user_data is not. Shouldn't we
> always annotate user_data of GAsyncCallback with (allow-none) as well?

Yes, I think user_data generally should be (allow-none). Curiously, the .gir does not have an explicit allow-none="1" for the user_data parameter, I guess that's implied because the "callback" parameter points to it as closure="2".

So I'm not quite sure what the bug is here. "None" seems to work fine, or does the program misbehave in a different way than an error message/crashing?

Perhaps the confusion is that "allow specifying None" is not the same as "skip specifying the argument entirely"? gobject-introspection, and hence pygobject, currently does not support default arguments (bug 558620). If we had default arguments, then we could indeed mark trailing "allow-none"s as "default to None", and "manager.prepare_async(None)" would work.

Is this what you mean?

Thanks!
Comment 4 Guillaume Desmottes 2012-04-25 07:38:08 UTC
(In reply to comment #3)
> >     #manager.prepare_async(None, None, None)
> 
> This does seem to work for me, the program is running without an error message.
> Can you please clarify what's broken here?

Indeed it works; it was just to illustrate the workaround we use atm to make this kind of things work.

> > The callback is allow-none (bug #653484) but the user_data is not. Shouldn't we
> > always annotate user_data of GAsyncCallback with (allow-none) as well?
> 
> Yes, I think user_data generally should be (allow-none). Curiously, the .gir
> does not have an explicit allow-none="1" for the user_data parameter, I guess
> that's implied because the "callback" parameter points to it as closure="2".

So that's gir bug as the user_data of GAsynCallback should always be allow-none?

> So I'm not quite sure what the bug is here. "None" seems to work fine, or does
> the program misbehave in a different way than an error message/crashing?
> 
> Perhaps the confusion is that "allow specifying None" is not the same as "skip
> specifying the argument entirely"? gobject-introspection, and hence pygobject,
> currently does not support default arguments (bug 558620). If we had default
> arguments, then we could indeed mark trailing "allow-none"s as "default to
> None", and "manager.prepare_async(None)" would work.
> 
> Is this what you mean?

Yes, I would have assume to be able to skip the arg entierly (more Pythonic). So this bug depends on bug #558620 and may be a dup if you already have a "Support default args in pygobject" generic bug.
Comment 5 Martin Pitt 2012-04-25 07:42:19 UTC
> Yes, I would have assume to be able to skip the arg entierly (more Pythonic).
> So this bug depends on bug #558620 and may be a dup if you already have a
> "Support default args in pygobject" generic bug.

We don't have a pygobject bug for that yet, so let's just use this bug for it.

Thanks for clarifying!
Comment 6 Simon Feltman 2013-02-20 10:54:04 UTC
*** Bug 652136 has been marked as a duplicate of this bug. ***
Comment 7 Simon Feltman 2013-07-28 01:08:09 UTC
Created attachment 250283 [details] [review]
tests: Add tests for multiple utf8 args with allow-none

Add test function which accepts multiple allow-none utf8 arguments.
This can be used for testing a default of NULL for unsupplied
arguments annotated with allow-none.
Comment 8 Simon Feltman 2013-07-28 04:18:20 UTC
Created attachment 250285 [details] [review]
Cleanup invoke args and kwargs combiner code

This was accidentally committed when pushing another trivial change. Although it is a relatively simple cleanup, I would have like to get some eyes on it.
Comment 9 Simon Feltman 2013-07-28 04:30:28 UTC
Created attachment 250286 [details] [review]
Add support for default arguments annotated with allow-none

Use a place holder object for un-supplied arguments which are
annotated with allow-none. This is then used later during marshaling
to supply NULL to as the default.
Additionally support an implicit default for callback user_data
using the same technique.
Comment 10 Simon Feltman 2013-07-28 04:54:42 UTC
Created attachment 250287 [details] [review]
Add support for default arguments annotated with allow-none

Use a place holder object for un-supplied arguments which are
annotated with allow-none. This is then used later during marshaling
to supply NULL as the default.
Additionally support an implicit default for callback user_data
using the same technique.
Comment 11 Simon Feltman 2013-07-28 05:48:46 UTC
Created attachment 250288 [details] [review]
Remove overrides which supply callback user_data as a keyword arg

Remove TreeSortable.set_sort_func and set_default_sort_func overrides
now that user_data is always optional.
Comment 12 Simon Feltman 2013-07-28 07:23:56 UTC
Created attachment 250289 [details] [review]
docs: Add a keyword value of None for allow-none annotations

Update documentation generator for allow-none arguments and
user_data arguments to show a keyword value of None.
Add skip for GDestroyNotify closure arguments.
Comment 13 Simon Feltman 2013-07-29 04:02:29 UTC
Review of attachment 250287 [details] [review]:

(self review)

After some thought, I don't like the implementation because it exacerbates existing problems in the cache creation code. For instance, there are multiple arg count increments and a lot of related logic scattered around in a complex loop with continues everywhere. This patch just adds more of that. We need to refactor this loop prior to adding new features.
Comment 14 Martin Pitt 2013-07-29 04:20:37 UTC
Comment on attachment 250283 [details] [review]
tests: Add tests for multiple utf8 args with allow-none

Fine for me to get the test API in. It would be nice to document @a in the same way as @c, though, i. e. "must be 1".
Comment 15 Martin Pitt 2013-07-29 04:29:19 UTC
Comment on attachment 250285 [details] [review]
Cleanup invoke args and kwargs combiner code

This seems fine to me by visual inspection. Could you compare a "check.valgrind" run before/after to ensure that we don't miss ref leaks?
Comment 16 Martin Pitt 2013-07-29 04:30:42 UTC
Comment on attachment 250288 [details] [review]
Remove overrides which supply callback user_data as a keyword arg

I think we have quite some more overrides which can be dropped after the main patch goes in, but this is a good start. This depends on the main patch, of course. Good to commit after that lands.
Comment 17 Simon Feltman 2013-07-31 05:58:45 UTC
Created attachment 250517 [details] [review]
Move PyGI direction code into new function
Comment 18 Simon Feltman 2013-07-31 05:58:50 UTC
Created attachment 250518 [details] [review]
Replace callable arg cache array+len with GPtrArray
Comment 19 Simon Feltman 2013-07-31 05:58:52 UTC
Created attachment 250519 [details] [review]
Remove special case marshaling for instance arguments

Remove duplicate code for marshaling struct and objects for
instance arguments. Re-use individual cache marshalers for
structs and objects with the instance argument. This required
removal of passing GITypeInfo to the marshaler because it is
not available for instance arguments. Instead always assume
"is_pointer" for the instance argument by using the cache.
Comment 20 Simon Feltman 2013-08-06 08:29:47 UTC
Created attachment 250928 [details] [review]
gimarshalingtests: Add tests for mixed allow-none and implicit args

Add test method with an allow-none in the middle of non allow-none
arguments. Add test method with an array and its length argument
interleaved with allow-none arguments.
Comment 21 Simon Feltman 2013-08-06 08:33:53 UTC
Created attachment 250932 [details] [review]
cache refactoring: Add comments to callable cache structure

Add comments to count fields on _PyGICallableCache.
Comment 22 Simon Feltman 2013-08-06 08:33:56 UTC
Created attachment 250933 [details] [review]
cache refactoring: Move PyGI direction code into new function
Comment 23 Simon Feltman 2013-08-06 08:33:59 UTC
Created attachment 250934 [details] [review]
cache refactoring: Replace callable arg cache array+len with GPtrArray

Replace manual management of the C array holding individual
argument caches with usage of CPtrArray. This provides storage
of the array length along with item memory management.
Comment 24 Simon Feltman 2013-08-06 08:34:02 UTC
Created attachment 250935 [details] [review]
cache refactoring: Remove special case marshaling for instance arguments

Remove duplicate code for marshaling struct and objects for
instance arguments. Re-use individual cache marshalers for
structs and objects with the instance argument. This required
removal of passing GITypeInfo to the marshaler because it is
not available for instance arguments. Instead always assume
"is_pointer" for the instance argument by using the cache.
Comment 25 Simon Feltman 2013-08-06 08:34:05 UTC
Created attachment 250936 [details] [review]
cache refactoring: Use bit field for PyGIDirection instead of enum

This supports cleaner logic when testing the direction of
arguments due to the majority of these tests being along the
lines of: (direction == FROM_PYTHON || direction == BIDIRECTIONAL)
Which is replaced with: (direction & FROM_PYTHON)
Comment 26 Simon Feltman 2013-08-06 08:34:08 UTC
Created attachment 250937 [details] [review]
cache refactoring: Remove continue statements from _args_cache_generate

Remove continue and goto statements from the large loop within
_args_cache_generate. This simplifies the sharing of parts of
the loop for future refactoring.
Comment 27 Simon Feltman 2013-08-06 08:34:12 UTC
Created attachment 250938 [details] [review]
cache refactoring: Move variable declarations to blocks where they are used
Comment 28 Simon Feltman 2013-08-06 08:34:15 UTC
Created attachment 250939 [details] [review]
cache refactoring: Move argument count increments outside of main logic

Share the incrementing of argument counts (n_from_py_args and n_to_py_args)
outside of the main logic where arg caches are created or updated.
Comment 29 Simon Feltman 2013-08-06 08:34:19 UTC
Created attachment 250940 [details] [review]
cache refactoring: Cleanup array length argument marshaling

Add shared function: _arg_cache_array_len_arg_setup for use
with both to and from array marshaling setup. This function
consolidates all of the edge cases regarding array length setup
and removes the need for flagging arguments with
PYGI_META_ARG_TYPE_CHILD_NEEDS_UPDATE.
Comment 30 Simon Feltman 2013-08-06 08:34:22 UTC
Created attachment 250941 [details] [review]
cache refactoring: Move arg cache field assignments into _arg_cache_new
Comment 31 Simon Feltman 2013-08-06 08:34:26 UTC
Created attachment 250942 [details] [review]
Add support for default arguments annotated with allow-none

Use a place holder object for un-supplied arguments which are
annotated with allow-none. This is then used later during marshaling
to supply NULL as the default.
Additionally support an implicit default for callback user_data
using the same technique.
Comment 32 Simon Feltman 2013-08-21 03:44:22 UTC
Setting this as a blocker for bug #693405 (the refactoring patches, not the feature itself).
Comment 33 Martin Pitt 2013-08-21 05:59:57 UTC
Comment on attachment 250928 [details] [review]
gimarshalingtests: Add tests for mixed allow-none and implicit args

LGTM, please push.
Comment 34 Martin Pitt 2013-08-21 06:00:52 UTC
Comment on attachment 250932 [details] [review]
cache refactoring: Add comments to callable cache structure

Please feel free to just push such simple patches.
Comment 35 Martin Pitt 2013-08-21 06:18:04 UTC
Review of attachment 250934 [details] [review]:

Looks fine otherwise, please push after the corrections.

::: gi/pygi-cache.c
@@ +1193,3 @@
+    if (n_args >= 0) {
+        cache->args_cache = g_ptr_array_new_full (n_args, (GDestroyNotify) _pygi_arg_cache_free);
+        g_ptr_array_set_size (cache->args_cache, n_args);

the _set_size() call here is unnecessary, _new_full already pre-allocates n_args.

::: gi/pygi-invoke.c
@@ +307,3 @@
 
+    state->args_data = g_slice_alloc0 (_pygi_callable_cache_args_len (cache) * sizeof (gpointer));
+    if (state->args_data == NULL && _pygi_callable_cache_args_len (cache) != 0) {

Can it actually happen that there are zero args? The documentation of g_slice_alloc doesn't say what happens if the requested size is 0 (e. g. malloc() can return NULL for size==0, which is a possible code path).

So I suggest we test for len != 0 first and only then do the slice alloc and NULL check. g_slice_free1() below gets along fine with a NULL pointer, FTR.

@@ +412,2 @@
         GIArgument *c_arg;
+        PyGIArgCache *arg_cache = g_ptr_array_index (cache->args_cache, i);

This could use the new _pygi_callable_cache_get_arg() macro for consistency
Comment 36 Martin Pitt 2013-08-21 06:22:36 UTC
Review of attachment 250935 [details] [review]:

I don't see anything obviously wrong here, but I have never touched that part of the marshaller. I was thinking of bug 688242 which is somewhat related to that, but as this only drops the type info but keeps the transfer direction, it should be fine. Thanks!

::: gi/pygi-cache.c
@@ -1114,3 @@
 
-            if (arg_cache == NULL)
-                goto arg_err;

Why is that check obsolete?
Comment 37 Martin Pitt 2013-08-21 06:24:18 UTC
Review of attachment 250936 [details] [review]:

Nice idea!
Comment 38 Martin Pitt 2013-08-21 06:25:27 UTC
Review of attachment 250937 [details] [review]:

LGTM
Comment 39 Martin Pitt 2013-08-21 06:27:30 UTC
Review of attachment 250938 [details] [review]:

OK
Comment 40 Martin Pitt 2013-08-21 06:30:05 UTC
Review of attachment 250939 [details] [review]:

Personally I like it better to keep the increment right next to the place where arguments are created (i. e. as the current code does), I find that easier to read.
Comment 41 Martin Pitt 2013-08-21 06:32:16 UTC
Review of attachment 250940 [details] [review]:

LGTM.
Comment 42 Martin Pitt 2013-08-21 06:32:51 UTC
Review of attachment 250941 [details] [review]:

OK
Comment 43 Martin Pitt 2013-08-21 06:42:14 UTC
Review of attachment 250942 [details] [review]:

I did a relatively shallow review, looks mostly good to me. But let's get the cache refactoring into 3.9.91, and this "main" patch together with overrides cleanup into 3.11 early. Many thanks for working on this!

::: tests/test_everything.py
@@ +722,3 @@
+
+        def callback(userdata):
+            self.assertTrue(userdata is None)

Please don't use assertions in callbacks, as they won't actually stop the test suite. Instead, save the passed userdata in TestCallbacks.user_data and compare it in the main test.
Comment 44 Simon Feltman 2013-08-21 08:35:39 UTC
(In reply to comment #35)
> Review of attachment 250934 [details] [review]:
> 
> Looks fine otherwise, please push after the corrections.
> 
> ::: gi/pygi-cache.c
> @@ +1193,3 @@
> +    if (n_args >= 0) {
> +        cache->args_cache = g_ptr_array_new_full (n_args, (GDestroyNotify)
> _pygi_arg_cache_free);
> +        g_ptr_array_set_size (cache->args_cache, n_args);
> 
> the _set_size() call here is unnecessary, _new_full already pre-allocates
> n_args.

It pre-allocates but doesn't set the size which is different (and needed). We want an allocated array filled with NULL already sized. I thought the same thing at first and ended up needing to set the size, so I'll add a comment stating this.

> ::: gi/pygi-invoke.c
> @@ +307,3 @@
> 
> +    state->args_data = g_slice_alloc0 (_pygi_callable_cache_args_len (cache) *
> sizeof (gpointer));
> +    if (state->args_data == NULL && _pygi_callable_cache_args_len (cache) !=
> 0) {
> 
> Can it actually happen that there are zero args? The documentation of
> g_slice_alloc doesn't say what happens if the requested size is 0 (e. g.
> malloc() can return NULL for size==0, which is a possible code path).
>
> So I suggest we test for len != 0 first and only then do the slice alloc and
> NULL check. g_slice_free1() below gets along fine with a NULL pointer, FTR.

Sure, although this patch just replaces the n_args usage with the macro. There is a whole series of these which we would probably want to add len != 0 checks as well:
https://git.gnome.org/browse/pygobject/tree/gi/pygi-invoke.c?id=3.9.90#n302

So we can add explicit logic to this and _invoke_state_clear, or continue relying on however the g_slice_alloc0 and g_slice_free1 pairing works (which seems to be working in any case).

> @@ +412,2 @@
>          GIArgument *c_arg;
> +        PyGIArgCache *arg_cache = g_ptr_array_index (cache->args_cache, i);
> 
> This could use the new _pygi_callable_cache_get_arg() macro for consistency

Sure.
Comment 45 Simon Feltman 2013-08-21 08:43:19 UTC
(In reply to comment #36)
> Review of attachment 250935 [details] [review]:
> 
> I don't see anything obviously wrong here, but I have never touched that part
> of the marshaller. I was thinking of bug 688242 which is somewhat related to
> that, but as this only drops the type info but keeps the transfer direction, it
> should be fine. Thanks!

Yep. It seem like this portion of the code should become cleaner in the future  if we can access the "instance-parameter" portions currently in GIR with an API (we could hopefully use APIs instead of assuming values). 

> ::: gi/pygi-cache.c
> @@ -1114,3 @@
> 
> -            if (arg_cache == NULL)
> -                goto arg_err;
> 
> Why is that check obsolete?

It was duplicating the check right after the call to _arg_cache_new:
https://git.gnome.org/browse/pygobject/tree/gi/pygi-cache.c?id=3.9.90#n1099
Comment 46 Martin Pitt 2013-08-21 08:50:35 UTC
(In reply to comment #44)
> > the _set_size() call here is unnecessary, _new_full already pre-allocates
> > n_args.
> 
> It pre-allocates but doesn't set the size which is different (and needed). We
> want an allocated array filled with NULL already sized. I thought the same
> thing at first and ended up needing to set the size, so I'll add a comment
> stating this.

Ah, because henceforth it uses the set macro instead of g_ptr_array_add(). Perhaps the latter would be a bit cleaner, but indeed let's not change the logic too much. So this is good, thanks!

> Sure, although this patch just replaces the n_args usage with the macro.

I know, your patch doesn't introduce that problem, I just spotted that (already existing) problem during patch review.
Comment 47 Simon Feltman 2013-09-26 06:31:59 UTC
Comment on attachment 250932 [details] [review]
cache refactoring: Add comments to callable cache structure

Attachment 250932 [details] pushed as 83208bf - cache refactoring: Add comments to callable cache structure
Comment 48 Simon Feltman 2013-09-26 06:34:47 UTC
Comment on attachment 250933 [details] [review]
cache refactoring: Move PyGI direction code into new function

Attachment 250933 [details] pushed as 52ea3af - cache refactoring: Move PyGI direction code into new function
Comment 49 Simon Feltman 2013-09-26 07:09:58 UTC
Comment on attachment 250935 [details] [review]
cache refactoring: Remove special case marshaling for instance arguments

Attachment 250935 [details] pushed as d5925b7 - cache refactoring: Remove special case marshaling for instance arguments
Comment 50 Simon Feltman 2013-09-26 07:13:13 UTC
Comment on attachment 250936 [details] [review]
cache refactoring: Use bit field for PyGIDirection instead of enum

Attachment 250936 [details] pushed as 87ae14b - cache refactoring: Use bit field for PyGIDirection instead of enum
Comment 51 Simon Feltman 2013-09-26 07:21:28 UTC
Comment on attachment 250937 [details] [review]
cache refactoring: Remove continue statements from _args_cache_generate

Attachment 250937 [details] pushed as dbc2cf5 - cache refactoring: Remove continue statements from _args_cache_generate
Comment 52 Simon Feltman 2013-09-26 07:26:26 UTC
Comment on attachment 250938 [details] [review]
cache refactoring: Move variable declarations to blocks where they are used

Attachment 250938 [details] pushed as c9d8639 - cache refactoring: Move variable declarations to blocks where they are used
Comment 53 Simon Feltman 2013-09-26 10:14:24 UTC
Comment on attachment 250940 [details] [review]
cache refactoring: Cleanup array length argument marshaling

Attachment 250940 [details] pushed as cb7e731 - cache refactoring: Cleanup array length argument marshaling
Comment 54 Simon Feltman 2013-09-26 10:56:57 UTC
Comment on attachment 250941 [details] [review]
cache refactoring: Move arg cache field assignments into _arg_cache_new

Attachment 250941 [details] pushed as 03f531f - cache refactoring: Move arg cache field assignments into _arg_cache_new
Comment 55 Simon Feltman 2013-09-26 11:42:41 UTC
Attachment 250928 [details] pushed as 6c9e4d5 - gimarshalingtests: Add tests for mixed allow-none and implicit args
Comment 56 Simon Feltman 2013-09-26 11:47:56 UTC
Comment on attachment 250942 [details] [review]
Add support for default arguments annotated with allow-none

Attachment 250942 [details] pushed as 510789d - Add support for default arguments annotated with allow-none
Comment 57 Simon Feltman 2013-09-26 12:11:04 UTC
The main patch has landed but there is still some work to do here:

1. Doc strings need to be updated as shown in comment #12. However, I think we should take a different approach to this. Rather than duplicating the argument in/out list dispersion code in Python to match the internals. We should just expose the internals to Python making things always correct. This could be done with two functions on PyGICallableInfo (something like get_py_in_args and get_py_out_args) which look at the caches in and out arg lists and build a new list containing PyGIArgInfos for them.

2. In terms of user_data for callbacks, I think if it is not passed during a connect call, a value of None should NOT be passed to the connected callback. We currently have:

    def callback(None):
        pass
    obj.connect(callback)

Which I think is a little weird and would prefer:

    def callback():
        pass
    obj.connect(callback)

We can get away with this because current users of non-overridden callback attach functions are already passing None explicitly, which would still make its way into callbacks. This would allow us to remove the whole user data shimming in GLib.py (user_data_varargs_shim).

3. More refactoring of argument cache building. I'd like to eradicate all the side effects caused by _arg_cache_new beyond creating a single arg cache. It currently creates and modifies sibling arg caches and modifies the callable cache array. Basically _arg_cache_new should only be responsible for creating a single arg cache and filling it out. The function which orchestrates this (_args_cache_generate) is where the rest of it belongs.
Comment 58 Simon Feltman 2013-09-27 02:56:18 UTC
Created attachment 255894 [details] [review]
Don't pass None to callbacks when user data is not specified

For APIs which support a callback and optional user data,
don't pass the user data to the callback if it was not explicitly
specified when the callback was connected.
Comment 59 Simon Feltman 2013-09-27 03:31:34 UTC
Comment on attachment 250288 [details] [review]
Remove overrides which supply callback user_data as a keyword arg

We need to keep this around for compatibility, otherwise calls to this without specifying user data will not get None as the callback user data.
Comment 60 Simon Feltman 2013-09-27 03:32:39 UTC
Comment on attachment 250289 [details] [review]
docs: Add a keyword value of None for allow-none annotations

Better to expose the internal caches to Python than re-create the logic.
Comment 61 Simon Feltman 2013-09-28 09:35:41 UTC
Created attachment 255971 [details] [review]
Refactor GLib.child_watch_add to make it more testable

Break the argument munging code into a separate function which
can be tested in isolation of adding a child watch. Update tests
to reflect this. Add additional failing test which specify
all args as keywords which we eventually need to support for
consistency with the rest of PyGObject.
Comment 62 Simon Feltman 2013-09-28 09:35:52 UTC
Created attachment 255972 [details] [review]
Refactor GLib.io_add_watch to make it more testable

Break the argument munging code into a separate function which
can be tested in isolation of adding an io watch.
Add additional failing test which specifies all args as keywords
which we eventually need to support for consistency with the
rest of PyGObject.
Comment 63 Simon Feltman 2013-09-28 09:35:56 UTC
Created attachment 255973 [details] [review]
Add support for variable user data arguments

Support a variable number of user data arguments for all callback
connection function where the user data is the last explicit argument.
This adds convience as well as consistency with the rest of PyGObject.
Cleanup overrides for GLib.idle_add, timeout_add, timeout_add_seconds,
io_add_watch, and child_watch_add which manually implemented this
feature.
Comment 64 Martin Pitt 2013-10-07 10:55:39 UTC
Review of attachment 255894 [details] [review]:

::: tests/test_everything.py
@@ +718,3 @@
         self.assertEqual(TestCallbacks.called, 100)
 
+    def test_callback_userdata_no_user_data(self):

The code looks fine to me, but I'm not happy about dropping/changing an existing test case. We should still retain a test for explicitly passing None for userdata and making sure it gets all the way through. Could you perhaps add this as a new test instead? Or do we already have a different test case which covers above case?
Comment 65 Martin Pitt 2013-10-07 10:55:44 UTC
Review of attachment 255894 [details] [review]:

::: tests/test_everything.py
@@ +718,3 @@
         self.assertEqual(TestCallbacks.called, 100)
 
+    def test_callback_userdata_no_user_data(self):

The code looks fine to me, but I'm not happy about dropping/changing an existing test case. We should still retain a test for explicitly passing None for userdata and making sure it gets all the way through. Could you perhaps add this as a new test instead? Or do we already have a different test case which covers above case?
Comment 66 Martin Pitt 2013-10-07 10:59:42 UTC
Review of attachment 255971 [details] [review]:

Testing the factored "munge input arguments" function is nice indeed. But can you please retain one test case which actually calls GLib.child_watch_add() with all the bells, whistles, and main loops, to ensure the new function is correctly integrated into that override?
Comment 67 Martin Pitt 2013-10-07 11:00:09 UTC
Review of attachment 255971 [details] [review]:

Testing the factored "munge input arguments" function is nice indeed. But can you please retain one test case which actually calls GLib.child_watch_add() with all the bells, whistles, and main loops, to ensure the new function is correctly integrated into that override?
Comment 68 Martin Pitt 2013-10-07 11:01:57 UTC
Review of attachment 255972 [details] [review]:

::: gi/overrides/GLib.py
@@ +718,3 @@
+    return real_channel, priority_, condition, func_fdtransform, user_data
+
+__all__.append('_io_add_watch_get_args')

Could we avoid making it part of the public API? I believe if a test would explicitly import that symbol it wouldn't need to be in __all__.
Comment 69 Simon Feltman 2013-10-07 23:59:42 UTC
(In reply to comment #64)
> Review of attachment 255894 [details] [review]:
> 
> ::: tests/test_everything.py
> @@ +718,3 @@
>          self.assertEqual(TestCallbacks.called, 100)
> 
> +    def test_callback_userdata_no_user_data(self):
> 
> The code looks fine to me, but I'm not happy about dropping/changing an
> existing test case. We should still retain a test for explicitly passing None
> for userdata and making sure it gets all the way through. Could you perhaps add
> this as a new test instead? Or do we already have a different test case which
> covers above case?

Yes, we already have tests for explicitly passing None. The modified test here was from a prior commit within this ticket. In this sense, it is an API change but only within a week or so. Which was support for implicit NULL as user_data:

def callback(user_data):
    pass
foo.add_callback(callback)

These newer patches change tail end user_data to work like vaargs. If you don't pass any user_data when adding a callback, you get nothing send to the callback as user_data:

def callback():
    pass
foo.add_callback(callback)

The former was feature was only committed on Sep. 26 so the API change is short lived:
https://git.gnome.org/browse/pygobject/commit/?id=510789d52e9e2f
Comment 70 Simon Feltman 2013-10-08 00:11:32 UTC
(In reply to comment #67)
> Review of attachment 255971 [details] [review]:
> 
> Testing the factored "munge input arguments" function is nice indeed. But can
> you please retain one test case which actually calls GLib.child_watch_add()
> with all the bells, whistles, and main loops, to ensure the new function is
> correctly integrated into that override?

I left two of them in (test_child_watch_no_data and test_child_watch_with_data):
https://git.gnome.org/browse/pygobject/tree/tests/test_subprocess.py?id=3.10.0#n78

Sorry to waste your very much appreciated review time... I'll try to add more post commit comment notes next time. But really I think the problem is our review tools don't give enough code context :(
Comment 71 Simon Feltman 2013-10-08 00:30:21 UTC
(In reply to comment #68)
> Review of attachment 255972 [details] [review]:
> 
> ::: gi/overrides/GLib.py
> @@ +718,3 @@
> +    return real_channel, priority_, condition, func_fdtransform, user_data
> +
> +__all__.append('_io_add_watch_get_args')
> 
> Could we avoid making it part of the public API? I believe if a test would
> explicitly import that symbol it wouldn't need to be in __all__.

Unfortunately this isn't the case:

>>> from gi.repository.GLib import _io_add_watch_get_args
ImportError: cannot import name _io_add_watch_get_args

>>> from gi.repository import GLib
>>> GLib._io_add_watch_get_args
AttributeError: 'gi.repository.GLib' object has no attribute '_io_add_watch_get_args'

>>> getattr(GLib, '_io_add_watch_get_args')
AttributeError: 'gi.repository.GLib' object has no attribute '_io_add_watch_get_args'


The only option I can think of here is add something like:

class _private(object):
    @staticmethod
    def _io_add_watch_get_args(...):
        pass

Or just obfuscating the name a bit more: _private_io_add_watch_get_args
We definitely need access to this stuff to do proper unittesting. I'll leave further obfuscation to a another commit because I recall needing to add internal functions to __all__ in other cases as well. We can take a pass at auditing the overrides once we have some type of agreed convention (but it shouldn't be much beyond the two I've added with this patch set).
Comment 72 Simon Feltman 2013-10-08 00:54:36 UTC
Attachment 255894 [details] pushed as 73c6213 - Don't pass None to callbacks when user data is not specified
Attachment 255971 [details] pushed as bc780ed - Refactor GLib.child_watch_add to make it more testable
Attachment 255972 [details] pushed as 549f849 - Refactor GLib.io_add_watch to make it more testable
Comment 73 Martin Pitt 2013-10-08 05:36:26 UTC
(In reply to comment #69)
> Yes, we already have tests for explicitly passing None. The modified test here
> was from a prior commit within this ticket.

Splendid, thanks. Should have checked myself..
> I left two of them in (test_child_watch_no_data and
> test_child_watch_with_data):
> https://git.gnome.org/browse/pygobject/tree/tests/test_subprocess.py?id=3.10.0#n78

Ack, thanks!

(In reply to comment #71)
> (In reply to comment #68)
> > Could we avoid making it part of the public API? I believe if a test would
> > explicitly import that symbol it wouldn't need to be in __all__.
> 
> Unfortunately this isn't the case:

Too bad. I probably mixed that up with Perl, where that works. So let's not worry about obfuscating it more, that only makes the code harder to read.
Comment 74 Martin Pitt 2013-10-14 08:03:08 UTC
Comment on attachment 255973 [details] [review]
Add support for variable user data arguments

I haven't scrutinized the logic very much, but the general idea and structure seem fine to me. We have good test case coverage in that area, too.

Many thanks!
Comment 75 Simon Feltman 2013-10-14 21:58:42 UTC
Comment on attachment 255973 [details] [review]
Add support for variable user data arguments

The original idea of supporting NULL defaults in ticket has been committed 
along with some extras (vaarg user data). General defaults from GI still 
needs completion of bug 558620. A defaults architecture has also been setup 
in PyGI and it should be easy to plug in the results of bug 558620. 
Leaving open as a general ticket for handling default arg values.

Attachment 255973 [details] pushed as 9456e83 - Add support for variable user data arguments
Comment 76 Simon Feltman 2013-10-19 10:13:36 UTC
Comment on attachment 250289 [details] [review]
docs: Add a keyword value of None for allow-none annotations

Unmarking as obsolete. I might end up going with this patch because it works well enough for docs.
Comment 77 Simon Feltman 2013-10-20 04:22:52 UTC
Comment on attachment 250289 [details] [review]
docs: Add a keyword value of None for allow-none annotations

Pushed since this was previously accepted and works well for the time being.

Attachment 250289 [details] pushed as ac776da - docs: Add a keyword value of None for allow-none annotations
Comment 78 GNOME Infrastructure Team 2018-01-10 20:06:34 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/11.