This is an archive of the discontinued Mercurial Phabricator instance.

repository: define interface for local repositories
ClosedPublic

Authored by indygreg on Mar 22 2018, 12:09 AM.

Details

Summary

Per discussions on the mailing list and at the 4.4 and 4.6 sprints,
we want to start defining interfaces for local repository primitives
so that we a) have a better idea of what the formal interface for
various types is b) can more easily introduce alternate implementations
of various components (e.g. in Rust).

We have previously implemented interfaces that declare the peer and
wire protocol APIs using the abc module.

This commit introduces a monolithic interface for the localrepository
class. It uses zope.interface - not abc - for defining and declaring
the interface.

The newly defined "completelocalrepository" interface is objectively
horrible. It is based on what is actually in localrepository and
doesn't represent a reasonable interface definition IMO. There's lots
of... unwanted garbage in the interface. In other words, it reflects
the horrible state of the localrepository "god object." But this is
fine: a goal of this commit is to get the interface defined so that
we have an interface. Future commits can refactor the interface
into sub-interfaces, remove unwanted public attributes, etc.

I attempted to define reasonable docstrings for the various interface
members. But there are so many of them and I didn't know what some are
used for. So I was lazy in a number of places and didn't write
docstrings or detailed usage docs.

Also, the members of the interface are defined in the order they are
declared in localrepo.py. This revealed that the grouping of things
in localrepo.py is... odd.

The localrepository class now declares that it implements our newly
defined interface. Unlike abc, zope.interface doesn't check interface
conformance at type creation time (abc uses metaclass magic to
validate interface conformance when a type is created - usually at
module import time). It does provide some functions for validating
class and object conformance with declared interfaces. We add these
checks to test-check-interfaces.py. We /could/ validate at run-time.
But we hold off for now. (I'm a bit scared of doing that because of
the various ways extensions monkeypatch repo instances.)

After this commit, test-check-interfaces.py will fail if the set of
public attributes on the localrepository class or instances change
without corresponding updates to the interface. This is by design.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Mar 22 2018, 12:09 AM
durin42 added inline comments.
mercurial/repository.py
273

I'm unclear: are all the methods supposed to be abstract, or are some of them default implementations?

484

Is this really the right default impl?

indygreg marked 2 inline comments as done.Mar 30 2018, 1:17 PM
indygreg added inline comments.
mercurial/repository.py
273

In zope.interface, the interface declaration is its own type which is completely independent from the implementation. This is different from abc, where the implementation inherits the abstract base class.

This means you can't have default implementations in the interface. Also, there is no self argument on methods. That's kind of wonky and confuses my IDE. But it does work.

484

No. I just didn't feel like adding a docstring because I didn't feel like researching what this is used for :)

indygreg marked 2 inline comments as done.Mar 30 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.