Re: [PATCH v2] emacs: Add customization group for replying

[Michal Sojka <sojkam1@fel.cvut.cz>]

On Thu, Sep 06 2012, Tomi Ollila wrote:
> On Thu, Sep 06 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
>
>>> So far good, but...
>>>
>>> You forgot to handle the current group setting for 
>>> notmuch-message-replied-tags -- currently notmuch-message.el has this:
>>>
>>> (defcustom notmuch-message-replied-tags '("replied")
>>>   "Tags to be automatically added to or removed from a message when it is
>>>   replied to.
>>> Any tag in the list will be added to a replied message or,
>>> if it is prefaced with a \"-\", removed.
>>>
>>> For example, if you wanted to add a \"replied\" tag and remove
>>> the \"inbox\" and \"todo\", you would set
>>>     (\"replied\" \"-inbox\" \"-todo\"\)"
>>>   :type 'list
>>>   :group 'notmuch-send)
>>>
>>> If the above was changed to 'normuch-send, (require 'notmuch-lib)
>>> was added to the beginning of notmuch-message.el and the defgroup
>>> below was written as
>>>
>>> (defgroup notmuch-reply
>>>   '((message-insertion custom-group)))
>>>    "Replying to messages.
>>>     ...
>>>
>>> would that work as expected ?
>>
>> Hi Tomi,
>>
>> I don't really get what are you trying to say. Do you mean that
>> notmuch-message-replied-tags is referenced from notmuch-lib.el without
>> (require 'notmuch-message)? I'm not sure whether it is required, but it
>> works here without problems.
>
> Ok, some sillines in my part and something I did not check well
> enough before replying...
>
> First, notmuch-lib.el defines notmuch-send group, yet 
> (defcustom notmuch-message-replied-tags ...) is defined 
> in notmuch-message.el which is not requiring notmuch-lib...
> and all of this works now...
>
> Actually, notmuch-lib.el defines all defgroups (except coolj).
>
> Then, I found the following: notmuch-mua-send-hook is defined 
> both in notmuch-hooks and notmuch-send groups (having a precedent
> helps a bit there when choosing alternatives below)
>
>
> All the defgroups are defined using pattern:
>
> (defgroup notmuch-<group> nil ...)
>
> We should follow this pattern unless there is good reason not to 
> do so here: notmuch-send is written as:
>
>   (defgroup notmuch-send nil
>     "Sending messages from Notmuch."
>     :group 'notmuch)
>
>   (custom-add-to-group 'notmuch-send 'message 'custom-group)
>
> But, as the documentation in this notmuch-reply definition references
> 'Message insertion' group, doing
>
>   (defgroup notmuch-reply 
>     '((message-insertion custom-group))'
>     "Replying to messages.
>
>   Most of the reply customization settings can be found in `Message
>   insertion' group. Notmuch specific settings are included directly
>   here."
>   :group 'notmuch)
>
> could be good thing to do here; definition matches doc from the beginning
> (just not adding any of the custom variables, yet).
>
>
> Now, currently notmuch-message.el defines notmuch-message-replied-tags
> and puts that to :group 'notmuch-send
>
> There are 2 alternatives:
>
> 1) change that to :group 'notmuch-reply

Now I understand. I think that 1) would be a good solution. I'll wait a
bit for other comments and than I'll send a new patch.

-Michal

>
> 2) add (custom-add-to-group 'notmuch-reply 'notmuch-message-replied-tags 'custom-variable)
>    to notmuch-message.el (or notmuch-lib.el)
>
> 2b) change :group 'notmuch-send -> :group 'notmuch-reply and
>    add (custom-add-to-group 'notmuch-send 'notmuch-message-replied-tags 'custom-variable)
>    to notmuch-message.el (or notmuch-lib.el)
>
>
> I personally would slightly lean to option 1 but if 2 gets more 
> supporters then 2b (so that the spesific group is in the group definition).
>
> On other words; 1 or 2b gets LGTM from me :)