"Jukka Zitting" <jukka.zitting@gmai...> wrote:

> Retrying...

Looping...


> Thanks, great work! There are however a number of points I'd like to
> raise.

That was the main idea of this commit :)

> See:
>
>     "Sets sitegroup's value."                <=
> midgard_connection_sitegroup_set()
>     "mgd MidgardConnection object"           <= MidgardConnection *mgd

I just follow GLib doc convention where variable name is defined first and object is
mentioned here because I had GObject in my mind while I wrote this.

> An improved doc comment would be something like this (see also my
> other comments about this method signature):
>
> /**
>  * \ingroup midgard_connection
>  * Sets the sitegroup of the given Midgard connection. The sitegroup
> of a connection
>  * affects all database accesses as described in the Midgard Sitegroup
> documentation.

Well... this is still confusing IMO. Main usage is designed for midgard-php
and we use request handler instead of real connection's one.

> > /* convention macros */
> > #define MIDGARD_TYPE_CONNECTION (midgard_connection_get_type())
> > [...]
> Why would MidgardConnection need to be a GObject? I see no use case for that.
>
> IMO a simple opaque struct would be much better:

1. I know you complained about lack of possibility to trigger any exception for java bindings.
2. I am still not sure if php's $_MIDGARD should be array.
3.
$_MIDGARD->set_lang($var);
$_MIDGARD->debug_start();
$_MIDGARD->errstr();

still seems to me much better than plenty of "other functions".

I just do not want to hear that something is not perfectly OO on midgard-php level
and I do not want to make OO bindings from GObjects and structures.

>     typedef struct _MidgardConnection MidgardConnection;
>
> Less code, less bugs... :-)

Yes and not. We still need to allocate and free memory for structures.
With GObject we do not need to provide special API , g_object_new
and g_object_unref should be enough.

> > typedef struct MidgardObjectProperty MidgardObjectProperty;
> > typedef struct MidgardObjectPropertyClass MidgardObjectPropertyClass;
>
> What are these doing here?

It wasn't typo. It was Chuck Norris ;)

> > extern MidgardConnection *midgard_connection_new(void);
>
> What can I do with an empty connection? IMO the
> midgard_connection_open() methods are good enough.

Isn't that convention for GObjects?

> > extern void midgard_connection_reset(MidgardConnection *mgd);
>
> Why is this needed?

This is very good question, and I ask myself about this too.

1. Internal members like sitegroup and lang can be "reset" in midgard-php's RINIT
instead of RSHUTDOWN. I spent some time to review legacy code and you must
say the same : this is chaotic. You can confuse yourself quite soon when you try
to understand midgard-core&midgard-php 1.x logic.
This function is proposed for midgard-php's RSHUTDOWN mainly.

2. Once you call mgd_auth_midgard which sets person member as GObject , you need
to free it at request end. This function should be smart about this because the same
internals are used for mgd_auth_midgard and Apache basic auth.

> > extern void midgard_connection_shutdown(MidgardConnection *mgd, gboolean free_config);
>
> I'd drop the free_config argument and rename the method to
> midgard_connection_close() (it rhymes better with
> midgard_connection_open()).

BTW, why do we close connection in RSHUTDOWN ?

> > extern void midgard_connection_sitegroup_set(MidgardConnection *mgd, GValue *value);
> > extern GValue *midgard_connection_sitegroup_get(MidgardConnection *mgd);
> > extern void midgard_connection_lang_set(MidgardConnection *mgd, GValue *value);
> > extern GValue *midgard_connection_lang_get(MidgardConnection *mgd);
>
> I believe the naming convention module_class_{set,get}_property() is
> more standard than ..._property_{set,get}().

OK.

> Why GValue? The more specific the API is the better. How about
> something like this:

Because GValue has type , so I could start to use it in 1.8 branch
and change 0 lines of API  part in 1.9/2.0.

>      * The given \c language parameter is the two-letter language code
> stored in the

Don't you think that one goes crazy when working with two different API  simultaneously? ;)
I know that no one is happy with necrocoding , but do not let me going mad ;)

But seriously , this can be done with ML *improvements*.

>
> > extern MidgardConfig *midgard_connection_config_get(MidgardConnection *mgd);
>
> I'd rather not have this. Is there any use case where the connection
> configuration is needed after the connection has been opened?

Blobdir for example. Personally I do not want to get blobdir as part of MidgardConnection API
as this is simple as logically as Midgard 1.x API.
All values from configuration file are MidgardConfig members no connection's one.

> > extern void midgard_connection_loglevel_set(MidgardConnection *mgd, const gchar *level);
> > extern guint midgard_connection_loglevel_get(MidgardConnection *mgd);
> > extern void midgard_connection_loghandler_set(
> >                 MidgardConnection *mgd, guint loghandler);
> > extern guint midgard_connection_loghandler_get(MidgardConnection *mgd);
>
> This functionality is already provided by GLib.

Yes, I do not want to change anything here. And I think you do not want to provide
public mgd structure as this is done now.

> The most we should
> provide is something like the following so clients wouldn't need to
> know the midgard-core log domain.

aha. right

>     guint midgard_set_log_handler(

wait wait :) did you notice midgard_set instead of midgard_connection_set ?
Was it Chuck Norris or intuitive API part ? ;)
You know what I mean :)

> All the other log functions are better implemented by the client application.

Yes, I removed log handler setting from core already.

Piotras

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@midg...
For additional commands, e-mail: dev-help@midg...


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@midg...
For additional commands, e-mail: dev-help@midg...