GNOME.org
 

Re: Removing empty keys from GrlData
On Tue, 29 Mar 2011 16:30:22 +0200, "Juan A." Suárez Romero <jasuarez igalia com> wrote: 
On Tue, 2011-03-29 at 14:06 +0000, Iago Toral wrote:

On Tue, 29 Mar 2011 14:07:39 +0200, "Juan A." Suárez Romero
> { "http://world.com/1.oth";, NULL, NULL }
> { "http://world.com/2.mp3";, "audio/mp3", NULL }
> { "http://world.com/3.unk";, NULL, NULL }
> { "http://world.com/4.ogg";, "audio/ogg", NULL }
> { "http://world.com/5.wav";, "audio/wav", NULL }
>
>
> So we have added those 2 related keys in our GrlData. Now the doubts 
>
> 1.a) What grl_data_has_key (data, GRL_METADATA_KEY_URL) should
> return?
> 1.b) What grl_data_has_key (data, GRL_METADATA_KEY_MIME) should
> return?
> 1.c) What grl_data_has_key (data, GRL_METADATA_KEY_BITRATE) should
> return?
>
> In current implementation, all functions return TRUE. The reason is 
> that
> this function returns TRUE if the asked key or any related key has a 
> value.

 I'd say that if we are forcing that all the keys must have a value,
 then has_key cannot return TRUE for BITRATE, because it does not
 actually have a value.
While I see coherent to return FALSE in the case of bitrate, I'm not so 
sure about the MIME either.

My doubt comes because in GrlData there are functions to handle
single-valued keys and functions to handle multi-valued keys.
The grl_data_has_key() was there since the very beginning, and depending 
on if it must handle single or multi-valued elements the behaviour
changes.

If we want grl_data_has_key() only deals with single-valued keys
(therefore meaning it would only check the first value), then it should 
return FALSE for MIME key.
If it must deal with multi-valued keys, then obviously it should return 
TRUE.
Single valued APIs are there just for the sake of simplifying common use cases, but this is a multi-valued system at its core and we should make our decisions based on that. for that I think we should return TRUE for mime. 


Now I'm wondering if we should keep the function to handle only
single-valued keys. For the case of multi-valued keys, user can either ask for all GrlRelatedKeys and then checking it related_keys to see if 
it has the key, or adding a new function like
grl_data_related_keys_has_key(). I feel a bit inclined to do this.
I think we would be complicating things more than necessary. We should think only of the case of multi-valued keys. As I said, the single-valued API is there only to simplify some common use cases.
This is a "test" function, so it is not big deal to only have one version that deals with all the values. People only caring about single-values can just forget about has_key and get the first value directly and test whether it is NULL or not. 




>
> 2) What grl_data_get_keys (data) should return?
>
> In current implementation, it returns the three keys:
> grl_data_get_keys() returns all keys in GrlData as well as the
> related
> keys.
>
> I think this has something to do with the work to do here: seems
> logical
> that bitrate shouldn't be returned, as it does not have any value.
>
> But what about mime? If get_keys() only checks the first value, then > clearly it should only return url. But if it checks all values, then 
> mime should also be returned.
>
> Thus, we should decide if this function should only check the first 
> value or not.
I think we should return the keys for which there is at least one known 
 value.
In the example I think it should return url and mime, but not bitrate.
This must be consistent with the behaviour of has_key(). If has_key(k) 
 returns TRUE get_keys() must return 'k' too.
I brought this question because, as it happened with grl_data_has_key(), 
grl_data_get_keys() were there to handle single-valued keys.

Now I wonder if we should change the semantic (and therefore possibly
breaking some client), or introducing a new function to get all keys
with some value (something like grl_data_get_all_keys()).
I think it is not necessary. We have time to add that in the future if we see the need for real. 

(...)


>
> 5.a) What grl_data_get_all_single_related_keys (data,
> GRL_METADATA_KEY_URL) should return?
> 5.b) What grl_data_get_all_single_related_keys (data,
> GRL_METADATA_KEY_MIME) should return?
> 5.c) What grl_data_get_all_single_related_keys (data,
> GRL_METADATA_KEY_BITRATE) should return?
>
>
> In current implementation (which btw doesn't honour to what
> documentation says), all of them returns the list with the 5 related 
> keys with their values.
>
> Now, I wonder if (5.a) shouldn't return a list with the 5 url values, 
> (5.b) a list with the 3 mimetypes values, and (5.c) an empty list.

 Well, wasn't this intended to just get all values for a key without
caring about related keys at all? In that case I guess it should do just that. That is, it should do what you suggest and not what is doing at 
 the moment.
Yes. In fact that is what the documentation states :) But not sure why I 
implemented in other way (probably an absentmindedness).
BTW, I don't like the name we chose for this at all, its behaviour is 
 not obvious from its name:
- Why the heck did we name this with the words "related_keys" in there 
 if this is supposed to ignore related keys all together?
- Why is this named _get_all_single_related_keys if this is supposed to 
 return values and not keys?

 I suggest we rename this to something more descriptive like:
 grl_data_get_single_values_for_key() or something along this line.



To be honest, I don't remember why we chose those names. And nobody
complained :)

In any case, I'll rename the functions.

BTW, in all this task, instead of directly removing/renaming the
functions, I'm marking them as "deprecated"; and my plan is to get rid 
of them in next or after next release.
For something we have just added a few days ago I don't see the need for that procedure, but I guess you propose it because the previous API is already in some release. Just wanted to say that probably there is no need for that, but I am ok with doing it anyway. 

Iago
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]