Patchwork [1,of,6] exthelper: add a cautionary note about adding attributes to containers

# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1545594763 18000
#      Sun Dec 23 14:52:43 2018 -0500
# Node ID 900b0af134505b6e5d227888212a17d8dd68342a
# Parent  0214962773921f15333f11d97212b307422aaaf5
exthelper: add a cautionary note about adding attributes to containers

On Wed, 26 Dec 2018 16:33:33 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1545594763 18000
> #      Sun Dec 23 14:52:43 2018 -0500
> # Node ID 900b0af134505b6e5d227888212a17d8dd68342a
> # Parent  0214962773921f15333f11d97212b307422aaaf5
> exthelper: add a cautionary note about adding attributes to containers

Queued, thanks.

> diff --git a/mercurial/exthelper.py b/mercurial/exthelper.py
> --- a/mercurial/exthelper.py
> +++ b/mercurial/exthelper.py
> @@ -255,6 +255,15 @@ class exthelper(object):
>          This function takes two arguments, the container and the name of the
>          function to wrap. The wrapping is performed during `uisetup`.
>  
> +        Adding attributes to a container like this is discouraged, because the
> +        container modification is visible even in repositories that do not
> +        have the extension loaded.  Therefore, care must be taken that the
> +        function doesn't make assumptions that the extension was loaded for the
> +        current repository.  For `ui` and `repo` instances, a better option is
> +        to subclass the instance in `uipopulate` and `reposetup` respectively.
> +
> +        https://www.mercurial-scm.org/wiki/WritingExtensions
> +
>          example::
>  
>              @eh.addattr(context.changectx, 'babar')

Can you update this example as well? Given most class instances belong to
ui or repo, I think only valid use of addattr() is to add an attribute to
module.

On Thu, 27 Dec 2018 06:45:26 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 26 Dec 2018 16:33:33 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1545594763 18000
>> #      Sun Dec 23 14:52:43 2018 -0500
>> # Node ID 900b0af134505b6e5d227888212a17d8dd68342a
>> # Parent  0214962773921f15333f11d97212b307422aaaf5
>> exthelper: add a cautionary note about adding attributes to containers
>
> Queued, thanks.
>
>> diff --git a/mercurial/exthelper.py b/mercurial/exthelper.py
>> --- a/mercurial/exthelper.py
>> +++ b/mercurial/exthelper.py
>> @@ -255,6 +255,15 @@ class exthelper(object):
>>          This function takes two arguments, the container and the name  
>> of the
>>          function to wrap. The wrapping is performed during `uisetup`.
>>
>> +        Adding attributes to a container like this is discouraged,  
>> because the
>> +        container modification is visible even in repositories that do  
>> not
>> +        have the extension loaded.  Therefore, care must be taken that  
>> the
>> +        function doesn't make assumptions that the extension was  
>> loaded for the
>> +        current repository.  For `ui` and `repo` instances, a better  
>> option is
>> +        to subclass the instance in `uipopulate` and `reposetup`  
>> respectively.
>> +
>> +        https://www.mercurial-scm.org/wiki/WritingExtensions
>> +
>>          example::
>>
>>              @eh.addattr(context.changectx, 'babar')
>
> Can you update this example as well? Given most class instances belong to
> ui or repo, I think only valid use of addattr() is to add an attribute to
> module.

It looks like the existing uses in evolve are to modify the peer classes,  
and lfs is using it to modify the context.basefilectx class.  So I see  
what you mean about belonging to the repo instance, but am also surprised  
at the same time.

Can you think of a use case for adding an attribute to a module?  I  
suppose it doesn't matter too much, as the existing example is contrived  
too.

On Thu, 27 Dec 2018 16:33:29 -0500, Matt Harbison wrote:
> On Thu, 27 Dec 2018 06:45:26 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> >>          example::
> >>
> >>              @eh.addattr(context.changectx, 'babar')
> >
> > Can you update this example as well? Given most class instances belong to
> > ui or repo, I think only valid use of addattr() is to add an attribute to
> > module.
> 
> It looks like the existing uses in evolve are to modify the peer classes,  
> and lfs is using it to modify the context.basefilectx class.  So I see  
> what you mean about belonging to the repo instance, but am also surprised  
> at the same time.
> 
> Can you think of a use case for adding an attribute to a module?  I  
> suppose it doesn't matter too much, as the existing example is contrived  
> too.

Actually I want to remove addattr() since I can't think of any valid use
case. We do hack class attributes in some extensions, but that's documented
to not do.