[PATCH 1 of 4 V2] flagutil: introduce a flagprocessorsmixin class

[Augie Fackler]

On Thu, Sep 05, 2019 at 08:43:16AM +0900, Yuya Nishihara wrote:
> On Mon, 2 Sep 2019 15:03:00 +0200, Pierre-Yves David wrote:
> > On 8/31/19 5:02 AM, Yuya Nishihara wrote:
> > > On Sun, 11 Aug 2019 13:10:47 +0200, Pierre-Yves David wrote:
> > >> # HG changeset patch
> > >> # User Pierre-Yves David <pierre-yves.david at octobus.net>
> > >> # Date 1565219568 -7200
> > >> #      Thu Aug 08 01:12:48 2019 +0200
> > >> # Node ID e6d22ec889461dbe819e0dbf4d2f0167736b2416
> > >> # Parent  6d61be152c5515fed315885f6c9cf9defe73de71
> > >> # EXP-Topic flag-processors
> > >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> > >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r e6d22ec88946
> > >> flagutil: introduce a flagprocessorsmixin class
> > >
> > > CC +indygreg
> > >
> > >> diff --git a/mercurial/revlogutils/flagutil.py b/mercurial/revlogutils/flagutil.py
> > >> --- a/mercurial/revlogutils/flagutil.py
> > >> +++ b/mercurial/revlogutils/flagutil.py
> > >> @@ -78,3 +78,74 @@ def insertflagprocessor(flag, processor,
> > >>           msg = _("cannot register multiple processors on flag '%#x'.") % (flag)
> > >>           raise error.Abort(msg)
> > >>       flagprocessors[flag] = processor
> > >> +
> > >> +class flagprocessorsmixin(object):
> > >> +    """basic mixin to support revlog flag processing
> > >> +
> > >> +    Make sure the `_flagprocessors` attribute is set at ``__init__`` time.
> > >
> > > I feel a plain function taking (revlog, flagprocessors, ...) is simpler
> > > than the mixin class which implicitly depends on revlog attributes. But
> > > maybe I'm biased. I don't like class inheritance in general.
> >
> > Are you suggesting (revlog, currently_supported_flags, …)? If so, I am
> > afraid this would lead to wrong API usage.
>
> Probably yes.
>
> > It is important that the
> > revlog bear the supported flags to help enforcing extension/config
> > isolation.
>
> Anyway, the user of the mixin would have to set self._flagprocessors
> properly. I don't see much difference between mixin + attributes and
> passing it as a function argument.
>
> > In all case, can we move forward with this series and adjust in a follow
> > up, I have a lot of other changes waiting on this ?
>
> If someone else agree with this change, please go ahead and queue this.
> I only have a _minor_ concern, and I don't have expertise around this code.

I'm also -0 on the mixin. Honestly I'm -0 on mixins in general,
especially with the direction I'd like to push remotefilelog and other
storage layers.

How does this interact with sqlitestore? It looks like as long as
self._flagprocessors is defined the right thing will happen, so could
we structure this to use composition rather than inheritance?

(If I get some decent answers on this I might queue it, but as it
stands this feels a little too much like a step backwards when it
comes to separation of concerns.)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel