launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'turnip/api/store.py'
> --- turnip/api/store.py	2015-02-26 21:34:02 +0000
> +++ turnip/api/store.py	2015-03-01 22:30:26 +0000
> @@ -6,6 +6,15 @@
>  import pygit2
>  
>  
> +def get_ref_type_name(ref_type_code):
> +    """Returns human readable ref type from ref type int."""
> +    types = {1: 'commit',
> +             2: 'tree',
> +             3: 'blob',
> +             4: 'tag'}
> +    return types.get(ref_type_code)

I'd use the GIT_OBJ_* constants rather than hardcoding the integers. Unlikely to change, but still.

> +
> +
>  class Store(object):
>      """Provides methods for manipulating repos on disk with pygit2."""
>  
> @@ -29,3 +38,38 @@
>          except (IOError, OSError):
>              print('Unable to delete repository from {}.'.format(repo))
>              raise
> +
> +    @staticmethod
> +    def get_refs(repo_path):
> +        """Return all refs for a git repository."""
> +        try:
> +            repo = pygit2.Repository(repo_path)
> +        except pygit2.GitError:
> +            print('Unable to locate repository in {}.'.format(repo))
> +            raise

I'd factor this out into a separate repo open method. It should also probably stop printing -- if the exception isn't descriptive enough, we should make it descriptive enough.

> +        ref_list = []
> +        for ref in repo.listall_references():
> +            git_object = repo.lookup_reference(ref).get_object()
> +            ref_list.append(
> +                {"ref": ref,
> +                 "object": {'sha': str(git_object.oid),

str works, but this should probably be git_object.oid.hex instead.

> +                            'type': get_ref_type_name(git_object.type)}})
> +        return ref_list

This could be a dict mapping ref name to {"object": "deadbeef", "type": "foo"}.

> +
> +    @staticmethod
> +    def get_ref(repo_path, ref):
> +        """Return a specific ref for a git repository."""
> +        try:
> +            repo = pygit2.Repository(repo_path)
> +        except pygit2.GitError:
> +            print('Unable to locate repository in {}.'.format(repo))
> +            raise
> +        try:
> +            git_object = repo.lookup_reference('refs/' + ref).get_object()
> +        except pygit2.GitError:
> +            print('Ref {} not found.'.format(ref))
> +            raise

"refs/" is technically part of the ref name. In terms of URL parsing it might be handled specially, but this method should not prepend "refs/".

This is another case where we should ensure a good exception is raised rather than printing.

> +        ref = {"ref": ref,
> +               "object": {'sha': str(git_object.oid),
> +                          'type': get_ref_type_name(git_object.type)}}
> +        return ref
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-02-25 07:28:44 +0000
> +++ turnip/api/tests/test_api.py	2015-03-01 22:30:26 +0000
> @@ -4,6 +4,7 @@
>  
>  import json
>  import os
> +import re
>  import unittest
>  import uuid
>  
> @@ -15,6 +16,7 @@
>  from webtest import TestApp
>  
>  from turnip import api
> +import test_helpers

Implicit relative imports don't exist in Python 3. We usually prefer absolute imports, but "import .test_helpers" would also work.

>  
>  
>  class ApiTestCase(TestCase):
> @@ -27,8 +29,15 @@
>          self.repo_path = str(uuid.uuid1())
>          self.repo_store = os.path.join(repo_store, self.repo_path)
>  
> -    def test_repo_post(self):
> -        resp = self.app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +    def get_ref(self, ref):
> +        test_helpers.init_repo(self.repo_store, commits=True, tags=True)
> +        resp = self.app.get('/repo/{}/refs/{}'.format(
> +            self.repo_path, ref))
> +        return json.loads(resp.json_body)
> +
> +    def test_repo_create(self):
> +        resp = self.app.post('/repo', json.dumps(
> +            {'repo_path': self.repo_path}))
>          self.assertEqual(resp.status_code, 200)
>  
>      def test_repo_delete(self):
> @@ -37,6 +46,32 @@
>          self.assertEqual(resp.status_code, 200)
>          self.assertFalse(os.path.exists(self.repo_store))
>  
> +    def test_repo_get_refs(self):
> +        """Ensure expected ref objects are returned and shas match."""
> +        ref = 'refs/heads/master'
> +        repo = test_helpers.init_repo(self.repo_store, commits=True, tags=True)
> +        resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
> +        body = json.loads(resp.json_body)
> +
> +        self.assertTrue(any(obj['ref'] == ref for obj in body))
> +        self.assertTrue(
> +            any(re.match('refs/tags.*', obj['ref']) for obj in body))
> +
> +        oid = str(repo.head.get_object().oid)  # git object sha
> +        resp_sha = ([obj for obj in body if obj['ref'] ==
> +                     ref][0]['object'].get('sha'))
> +        self.assertEqual(oid, resp_sha)

These assertions become a lot prettier if the refs collection returns a dict.

> +
> +    def test_repo_get_ref(self):
> +        ref = 'heads/master'
> +        resp = self.get_ref(ref)
> +        self.assertEqual(ref, resp['ref'])
> +
> +    def test_repo_get_tag(self):
> +        tag = 'tags/test-tag'
> +        resp = self.get_ref(tag)
> +        self.assertEqual(tag, resp['ref'])
> +
>  
>  if __name__ == '__main__':
>      unittest.main()
> 
> === added file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/tests/test_helpers.py	2015-03-01 22:30:26 +0000
> @@ -0,0 +1,37 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +from pygit2 import (
> +    init_repository,
> +    GIT_OBJ_COMMIT,
> +    Signature,
> +    )
> +
> +AUTHOR = Signature('Test Author', 'author@xxxxxxx')
> +COMMITTER = Signature('Test Commiter', 'committer@xxxxxxx')
> +
> +
> +def create_commit(repo, parents=[]):
> +    tree = repo.TreeBuilder().write()
> +    commit = repo.create_commit(
> +        'refs/heads/master',
> +        AUTHOR, COMMITTER, 'test commit.',
> +        tree,
> +        parents  # parent
> +    )
> +    return commit
> +
> +
> +def create_tag(repo):
> +    oid = repo.head.get_object().oid
> +    tag = repo.create_tag(
> +        'test-tag', oid, GIT_OBJ_COMMIT, COMMITTER, 'test tag')
> +    return tag
> +
> +
> +def init_repo(repo_path, commits=None, tags=None):
> +    repo = init_repository(repo_path, True)
> +    if commits:
> +        create_commit(repo)
> +    if tags:
> +        create_tag(repo)
> +    return repo
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-02-26 21:34:02 +0000
> +++ turnip/api/views.py	2015-03-01 22:30:26 +0000
> @@ -1,5 +1,6 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
> +import json
>  import os
>  
>  from cornice.resource import resource
> @@ -10,7 +11,7 @@
>  from turnip.api.store import Store
>  
>  
> -@resource(collection_path='repo', path='/repo/{name}')
> +@resource(collection_path='/repo', path='/repo/{name}')
>  class RepoAPI(object):
>      """Provides HTTP API for repository actions."""
>  
> @@ -44,3 +45,35 @@
>              Store.delete(repo)
>          except Exception:
>              return exc.HTTPNotFound()  # 404
> +
> +
> +@resource(collection_path='/repo/{name}/refs',
> +          path='/repo/{name}/refs/{ref:.*}')
> +class RefAPI(object):
> +    """Provides HTTP API for git references."""
> +
> +    def __init__(self, request):
> +        config = TurnipConfig()
> +        self.request = request
> +        self.repo_store = config.get('repo_store')
> +
> +    def collection_get(self):
> +        name = self.request.matchdict['name']
> +        repo = os.path.join(self.repo_store, name)
> +        try:
> +            refs = Store.get_refs(repo)
> +        except Exception as ex:
> +            print(ex)
> +            return exc.HTTPNotFound()  # 404

It's probably worth factoring this out, possibly even into a decorator that turns the repo path into a repo object before it gets into the method. I'm not sure if that's a common pattern in cornice, though.

> +        return json.dumps(refs)
> +
> +    def get(self):
> +        name = self.request.matchdict['name']
> +        ref = self.request.matchdict['ref']
> +        repo = os.path.join(self.repo_store, name)
> +        try:
> +            ref = Store.get_ref(repo, ref)
> +        except Exception as ex:
> +            print(ex)
> +            return exc.HTTPNotFound()
> +        return json.dumps(ref)
> 


-- 
https://code.launchpad.net/~blr/turnip/api-refs/+merge/251394
Your team Launchpad code reviewers is subscribed to branch lp:turnip.