Page MenuHomePhabricator

merge: return an attrs class from update() and applyupdates()
ClosedPublic

Authored by indygreg on Mar 5 2018, 1:17 AM.

Details

Summary

Previously, we returned a tuple containing counts. The result of an
update is kind of complex and the use of tuples with nameless fields
made the code a bit harder to read and constrained future expansion
of the return value.

Let's invent an attrs-defined class for representing the result of
an update operation.

We provide getitem and len implementations for backwards
compatibility as a container type to minimize code churn.

In (at least) Python 2, the % operator seems to insist on using
tuples. So we had to update a consumer using the % operator.

.. api::

merge.update() and merge.applyupdates() now return a class
with named attributes instead of a tuple. Switch consumers
to access elements by name instead of by offset.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Mar 5 2018, 1:17 AM
indygreg updated this revision to Diff 6969.Mar 12 2018, 6:28 PM
martinvonz added inline comments.
mercurial/merge.py
1404–1405

mpm made me use a tuple with slots for scmutil.status. Any idea when that's better? I think it was something about performance, but we don't care about that here.

martinvonz accepted this revision.Mar 21 2018, 8:16 PM
This revision is now accepted and ready to land.Mar 21 2018, 8:16 PM

__slots__ allocates enough space for the exact set of attributes specified rather than backing object instances by __dict__. If you create thousands of small objects that all have the same fields, __slots__ can yield some performance wins.

durin42 accepted this revision.Mar 24 2018, 3:07 PM
This revision was automatically updated to reflect the committed changes.