Created attachment 279859 [details] [review]
doc: improve doc of g_subprocess_wait()

When using this API, I wasn't sure what the cancellable does. I think
it's generally desirable to kill the subprocess if the wait operation is
cancelled, since in this case the application is no longer interested by
the subprocess.

Created attachment 279860 [details] [review]
doc: possible error domains of GSubprocess

AFAIK the two possible error domains are G_SPAWN_ERROR and
G_SPAWN_EXIT_ERROR (the latter is used when calling
g_spawn_check_exit_status(), which is done by g_subprocess_wait_check()
for example).

The documentation is added to the main description of GSubprocess, not
for each individual functions.

Created attachment 279861 [details] [review]
doc: improve doc of g_input_stream_read()

I recently needed to nul-terminate the returned buffer, and I wasn't
sure if g_input_stream_read() does that or not. I've checked
glocalfileinputstream.c, which calls read(2) which doesn't nul-terminate
the buffer. So I assume it's the same behavior for all GInputStream
subclasses.

Created attachment 279862 [details] [review]
doc: small improvement and fixes

- Add an example to g_strsplit(), like it is done for g_strsplit_set().

- GTK-Doc generates a list if a "1." is at the beginning of a line.

Review of attachment 279859 [details] [review]:

thanks

Review of attachment 279860 [details] [review]:

Why do you need to know this?

In your opinion, if we add this here now, are we then constrained from returning new types of errors in the future?

Review of attachment 279861 [details] [review]:

Certainly doesn't hurt...

Review of attachment 279862 [details] [review]:

Thanks.  Kinda sucks about the '1.' business...

Thanks for the quick review.

(In reply to comment #6)
> Review of attachment 279860 [details] [review]:
> 
> Why do you need to know this?

It's for printing a clearer error message for G_SPAWN_ERROR_NOENT. Instead of:

> Failed to execute child process "foobar" (No such file or directory)

I print:

> foobar doesn't seem to be installed.

But the error can also be useful to execute a fallback command, or do some other post-processing. Or when the command doesn't exist, propose to install it with PackageKit.

> In your opinion, if we add this here now, are we then constrained from
> returning new types of errors in the future?

The sentence can be formulated differently, so the list of error domains is not restricted to those mentioned. For example:

> Currently the possible error domains are %G_SPAWN_ERROR and
> %G_SPAWN_EXIT_ERROR, but it can be extended in the future.

Or if you don't want API stability guarantee for error domains:
s/extended/extended or changed/

(In reply to comment #9)
> It's for printing a clearer error message for G_SPAWN_ERROR_NOENT. Instead of:
> 
> > Failed to execute child process "foobar" (No such file or directory)
> 
> I print:
> 
> > foobar doesn't seem to be installed.


How does this help?  You can already do this today...

This doesn't seem to be a clarifying statement so much as it is some kind of a new promise that you're looking for.  What's that promise?  That we will definitely emit a particular error code in a particular situation?

I've always seen the error domains and codes as part of the API, since they are documented and available in the API references. Executing a certain piece of code depending on the error type is useful. In POSIX C the errno and possible error types are also well documented.

But again, if you don't want API stability guarantee on error codes for GSubprocess, I can understand (since it's a quite new code, and that the old GLib API is not really useful anymore and will maybe be deprecated in the future, no?). So the other solution is to not document error domains, and developers needing them must look at the code.

All the patches are pushed, except the one about possible error domains for GSubprocess. No need to discuss at length this stuff, so I think this bug can be closed.