Edgewall Software

Opened 8 years ago

Last modified 8 years ago

#11610 closed enhancement

Move Changeset, Node and Repository classes to model.py — at Version 6

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: version control Version:
Severity: normal Keywords:
Cc: Jun Omae Branch:
Release Notes:

Git and Mercurial raise a TracError rather than showing a traceback when attempting to view added and deleted files (#9775).

API Changes:
  • Abstract base classes have abc.ABCMeta metaclass and use abstractmethod decorator on abstract methods.
  • Mock has support for abstract base classes defined using abc.ABCMeta.
  • A CachedChangeset instance rather than an instance of the abstract Changset class is passed to the repository change listeners.
Internal Changes:

Description

As discussed in #11609, move theChangeset, Node and Repository classes to a new model.py module in trac/versioncontrol.

Change History (6)

comment:1 by Ryan J Ollos, 8 years ago

Status: newassigned

comment:2 by Ryan J Ollos, 8 years ago

Looking it at it more closely now, it could be that the reason the classes are in api.py rather than model.py is that they are abstract classes.

comment:3 by Ryan J Ollos, 8 years ago

Some additional experimental changes in log:rjollos.git:t11610

The classes in the abc module were utilized in #11581. There are other classes in the repository which are made abstract by raising NotImplementedErrors in methods, but the ABCMeta class forces subclasses to adhere to the contract (comment:2:ticket:11581), and more clearly identifies the base classes as abstract.

Some unit tests are currently failing due to:

  1. GitRepository doesn't implemented get_path_history (#9775):
    ======================================================================
    ERROR: test_node_get_history_with_empty_commit (tracopt.versioncontrol.git.tests.PyGIT.NormalTestCase)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/user/Workspace/tracdev/teo-rjollos.git/tracopt/versioncontrol/git/tests/PyGIT.py", line 251, in test_node_get_history_with_empty_commit
        repos = self.env.get_repository('gitrepos')
      File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/env.py", line 567, in get_repository
        return RepositoryManager(self).get_repository(reponame)
      File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/api.py", line 575, in get_repository
        repoinfo.copy())
      File "/home/user/Workspace/tracdev/teo-rjollos.git/tracopt/versioncontrol/git/git_fs.py", line 280, in get_repository
        use_committer_time=self.use_committer_time,
    TypeError: Can't instantiate abstract class GitRepository with abstract methods get_path_history
    
  1. A Mock object can't be created directly from a base class with abstractmethods. It is possible that we need to modify Mock in order to deal with abstract base classes. I'm not sure yet what is the best approach to take on this.
    ======================================================================
    ERROR: test_clean_sync (trac.versioncontrol.tests.cache.CacheTestCase)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/tests/cache.py", line 158, in test_clean_sync
        youngest_rev=2)
      File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/versioncontrol/tests/cache.py", line 54, in get_repos
        next_rev=(lambda x: int(x) < youngest_rev and x + 1 \
      File "/home/user/Workspace/tracdev/teo-rjollos.git/trac/test.py", line 94, in Mock
        mock = cls(*initargs)
    TypeError: Can't instantiate abstract class Mock with abstract methods close, get_changes, get_changeset, get_node, get_oldest_rev, get_path_history, get_youngest_rev, next_rev, normalize_path, normalize_rev, previous_rev, rev_older_than
    

comment:4 by Ryan J Ollos, 8 years ago

Changeset is instantiated directly in CachedRepository. I fixed the failing test by instead instantiating the derived class. The only place I see the object used is: tags/trac-1.0.1/trac/versioncontrol/api.py@:662,674#L657. So now a GitChangeset, SubversionChangeset or other derived type is passed to the change listener. Are there any possible negative consequences to this that I'm not seeing?

As for creating Mock instances of ABCs, it seemed most straightforward to just remove the abstract methods from the class. Like any other methods in a mocked object, an AttributeError will be raised if methods omitted from the Mock are utilized in a function under test.

Latest changes added to log:rjollos.git:t11610.

in reply to:  4 comment:5 by Jun Omae, 8 years ago

Cc: Jun Omae added

Changeset is instantiated directly in CachedRepository. I fixed the failing test by instead instantiating the derived class. The only place I see the object used is: tags/trac-1.0.1/trac/versioncontrol/api.py@:662,674#L657. So now a GitChangeset, SubversionChangeset or other derived type is passed to the change listener. Are there any possible negative consequences to this that I'm not seeing?

I think it would be good to create CachedChangeset instance rather than Changeset. Also, sync_changeset method is for only cached repository. Then, GitChangeset and SubversionChangeset never be passed to the change listener.

See [058936ed/jomae.git].

As for creating Mock instances of ABCs, it seemed most straightforward to just remove the abstract methods from the class. Like any other methods in a mocked object, an AttributeError will be raised if methods omitted from the Mock are utilized in a function under test.

We could add support for ABCMeta class as base class of Mock. See [8f55ca19/jomae.git].

comment:6 by Ryan J Ollos, 8 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Those changes look good, thanks. Committed to trunk in [12782:12783].

We'll need to implement get_path_history in the MercurialPlugin's Repository subclass. I don't have the necessary permission to push to the branch.

  • tracext/hg/backend.py

    # HG changeset patch
    # User ryan.j.ollos
    # Date 1400115750 25200
    # Branch 1.0
    # Node ID 251cde2ad7fdddbfbde7542bccd2a2f4a129b9c9
    # Parent  21bb3cc19977ee1f5e546fc21f440ebfd6231404
    Implemented abstract method `get_path_history`, which currently displays just a `TracError`. Refs #11610, #9775.
    
    diff -r 21bb3cc19977 -r 251cde2ad7fd tracext/hg/backend.py
    a b  
    3333from trac.util import arity
    3434from trac.util.datefmt import FixedOffset, utc
    3535from trac.util.text import exception_to_unicode, shorten_line, to_unicode
    36 from trac.util.translation import domain_functions
     36from trac.util.translation import _, domain_functions
    3737from trac.versioncontrol.api import Changeset, Node, Repository, \
    3838                                    IRepositoryConnector, RepositoryManager, \
    3939                                    NoSuchChangeset, NoSuchNode
     
    747747    def get_youngest_rev(self):
    748748        return self.changectx().hex()
    749749
     750    def get_path_history(self, path, rev=None, limit=None):
     751        raise TracError(_("Unsupported \"Show only adds and deletes\""))
     752
    750753    def previous_rev(self, rev, path=''): # FIXME: path ignored for now
    751754        for p in self.changectx(rev).parents():
    752755            if p:
Last edited 8 years ago by Ryan J Ollos (previous) (diff)
Note: See TracTickets for help on using tickets.