Hi, I left my responses inline below: 

On 8/9/2015 10:53 AM, Kim Alvefur wrote:
> Hi!
> 
> Slowly reviewing, some nits, questions, feedback:
> 
> On 2015-07-13 17:16, Tylor Reynolds wrote:
>> mod_pubsub: fix registration of config-node and retrieve-default features
>> diff --git a/plugins/mod_pubsub/mod_pubsub.lua 
>> b/plugins/mod_pubsub/mod_pubsub.lua
>> index 40c28d2..ba9df31 100644
>> --- a/plugins/mod_pubsub/mod_pubsub.lua
>> +++ b/plugins/mod_pubsub/mod_pubsub.lua
>> @@ -68,11 +68,13 @@ local feature_map = {
>>      get_items = { "retrieve-items" };
>>      add_subscription = { "subscribe" };
>>      get_subscriptions = { "retrieve-subscriptions" };
>> -    set_configure = { "config-node" };
>> -    get_default = { "retrieve-default" };
>> +    set_node_config = { "config-node" };
>>  };
>>  
>>  local function add_disco_features_from_service(service)
>> +    -- not implemented by service, implemented by lib_pubsub
>> +    module:add_feature(xmlns_pubsub.."#retrieve-default");
>> +
> 
> Is there a reason for that instead of eg
> `node_defaults = { "retrieve-default" };` ?
> 
> Switching to hardcoded values when the preset seems to be automagic
> detection feels odd, but might be justifiable.

Yeah I didn't like the special casing either. In a later commit that I
haven't submitted yet (I was waiting for this feedback), I reimplemented
the automagic detection but using the lib pubsub instead of the pubsub 
service. Here's a link to the commit on github: 
https://github.com/tylorr/prosody/commit/f91cb7936e907bffc8b07601aa7eda21d9c2e0e4

I could try and rearrange and squash the commits together if you prefer
the newer one and don't want the original to stick around.

> 
> +     if not #actions then
> 
> This looks like a simple mistake in assuming that 0 in a boolean context
> is false.

Yeah, I need to watch out for that, most other languages I work with treat
0 as a false value.

> 
> +     local action_names = {};
> +     for _, action in ipairs(actions) do
> +             table.insert(action_names, action.name)
> +     end
> +
> +     local ns_ = namespace and namespace.."_" or "";
> +     local compound_action = table.concat(action_names, "_");
> +     local handler = handlers[stanza.attr.type.."_"..ns_..compound_action];
> +
>       if handler then
> -             handler(origin, stanza, action, service);
> +             if #actions <= 1 then actions = actions[1] end
> +             handler(origin, stanza, actions, service);
>               return true;
>       end
> 
> Do we really need support for an arbitrary number of actions at the same
> time?  Are there more than 2 anywhere?
> 

No, I guess I did it that way for future proofing in case there were ever
more actions (which is unlikely) and it easily handles the error cases
when there are too many actions.

Thank you for your feedback.


-- 
You received this message because you are subscribed to the Google Groups 
"prosody-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to prosody-dev+unsubscr...@googlegroups.com.
To post to this group, send email to prosody-dev@googlegroups.com.
Visit this group at http://groups.google.com/group/prosody-dev.
For more options, visit https://groups.google.com/d/optout.

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to