Closed
Bug 655942
Opened 13 years ago
Closed 12 years ago
[shipping] diff view should be clever for revisions not in the same repo
Categories
(Webtools Graveyard :: Elmo, defect, P2)
Webtools Graveyard
Elmo
Tracking
(Not tracked)
RESOLVED
FIXED
2.2
People
(Reporter: Pike, Assigned: Pike)
References
Details
(Keywords: dogfood)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
peterbe
:
review+
|
Details | Diff | Splinter Review |
The diff view currently enforces that the two revisions are in the same repository, which makes reviewing in our new branching scheme hard.
It's kinda easy to fix, as we have the data in the database, by finding the common ancestor with a code snippet like
cs=Changeset.objects.order_by('-pk').filter(repositories__name__endswith='aurora/'+loc).filter(repositories__name__endswith='2.0/'+loc)[0]
You have to find the repos for the changesets really first, the line above is just what I use right now, hard-coded to 2.0 vs aurora.
I'd like to see this soon.
Assignee | ||
Comment 2•12 years ago
|
||
This is a pretty deep patch queue that I'd prefer to land in patches, the better view than this patch is in https://github.com/Pike/elmo/compare/develop...feature%2Fbug-655942-multi-repo-diff
Requesting review here either way, but feel free to comment on the queue, too.
Attachment #634963 -
Flags: review?(peterbe)
Comment 3•12 years ago
|
||
Comment on attachment 634963 [details] [diff] [review]
one big patch for queue
Review of attachment 634963 [details] [diff] [review]:
-----------------------------------------------------------------
* The (ab)use of __init__.py is bad practice in my point of view.
* The RepoMixin class is not a mixin since it aligns itself in as an inheritable class and should thus be renamed.
* The repeated dbrepo() method needs to go.
Great work otherwise!! I'm not going deep into the understanding of some of the inner works of the tests but it looks promising all around.
::: apps/pushes/tests/__init__.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
I disagree about this file name. Importing stuff from a package's __init__ is error-prone and confusing. It's error-prone because it can lead to circular imports.
I suggest you create a new file called "base.py" (ie. apps/pushes/tests/base.py)
::: apps/pushes/tests.py
@@ +24,3 @@
>
>
> +class RepoMixin(TestCase):
Excellent! But is it really a mixin? Looks to me like it's acting as a base class.
I suggest you move this into tests/base.py and call it something like RepoTestBase.
@@ +44,5 @@
> + if name is None:
> + name = self.repo_name
> + repo = Repository.objects.create(
> + name=name,
> + url=urlpattern % name
Smart! I like it!
@@ +71,4 @@
> if self._old_repository_base is not None:
> settings.REPOSITORY_BASE = self._old_repository_base
>
> + def dbrepo(self, name=None, changesets_from=None,
I know splinter review is a bit messed up when you do a rename AND modifications.
However, looking at the code in git, it appears this method is repeated. The one in the RepoMixin looks identical.
Can you refactor to DRY?
::: apps/pushes/tests/test_pushes.py
@@ +6,5 @@
> +import shutil
> +import tempfile
> +import datetime
> +import time
> +try:
Why bother? json comes builtin in python 2.6. Just use that.
I think simplejson might be slightly faster (or something like that) which is not applicable in test suites.
@@ +26,5 @@
> +from pushes import repo_fixtures
> +from pushes.utils import get_or_create_changeset
> +from pushes.views.api import jsonify
> +from pushes.utils import handlePushes, PushJS
> +from . import mock_ui
Again, I would strongly recommend we use `from .base import mock_ui` instead
::: apps/pushes/views/diff.py
@@ +34,5 @@
> +
> +
> +class DiffView(View):
> +
> + def _universal_newlines(self, content):
You should prefix this with staticmethod. Since it's not related to the class from an OO point-of-view but it belongs from a structural point of view. Ie.
```
class DiffView(View):
@staticmethod
def _universal_newlines(content):
...
def get(self, request):
content = self._universal_newlines(request.content)
```
Or else, there's no reason for it to accept the `self` if it's not going to use it.
Attachment #634963 -
Flags: review?(peterbe) → review-
Assignee | ||
Comment 4•12 years ago
|
||
I've addressed the comments they're distributed among the various patch steps in the queue, notably, the test factor patch has the base.py change, and I also used the RepoTestBase in test_pushes now. Left the "copy of DiffTestCase" because that test uses the self.repo_name, self.repo, while the other test uses the network from the fixture code.
Also, the updated patch queue is pushed to my clone, the compare url is the same as before.
Attachment #634963 -
Attachment is obsolete: true
Attachment #635259 -
Flags: review?(peterbe)
Comment 5•12 years ago
|
||
Comment on attachment 635259 [details] [diff] [review]
big patch, with review comments
Review of attachment 635259 [details] [diff] [review]:
-----------------------------------------------------------------
Great work!
::: apps/pushes/tests/base.py
@@ +8,5 @@
> +from django.conf import settings
> +from mercurial.ui import ui as hg_ui
> +from test_utils import TestCase
> +from life.models import Repository
> +from ..utils import get_or_create_changeset
`pushes` is a package on the path so you can do `from pushes.utils import get_or_create_changeset`
Attachment #635259 -
Flags: review?(peterbe) → review+
Comment 6•12 years ago
|
||
Commits pushed to develop at https://github.com/mozilla/elmo
https://github.com/mozilla/elmo/commit/9e3a35ace5ab57c51f716466781d59ba4023d98f
bug 655942, first move to class-based view
https://github.com/mozilla/elmo/commit/8b336057cee05d38e3137b5ad830a7d705a21aef
bug 655942, factor some worker code of the diff view
https://github.com/mozilla/elmo/commit/d9371ab4b38fbfeb3355a9ce52708121b95852e8
bug 655942, factor pushes tests, no functional change
https://github.com/mozilla/elmo/commit/56b2e2b6cf9df3c520ac3735f3814306799dfd77
bug 655942, support diffs across repos, r=peterbe
There are extensive tests to make sure that the routines to
piece together the paths and copies from different repos match
what hg would return, re-using hg internal code as possible.
Assignee | ||
Comment 7•12 years ago
|
||
Marking FIXED. I've ended up leaving the ..utils in, but we'll not start doing that going forward, neither I not peter are strongly in favor of that.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → 2.2
Updated•4 years ago
|
Product: Webtools → Webtools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•