| Submitter | Adrian Buehlmann |
|---|---|
| Date | May 16, 2015, 8:51 a.m. |
| Message ID | <3d14c1217117795e3b79.1431766292@kork> |
| Download | mbox | patch |
| Permalink | /patch/9118/ |
| State | Accepted |
| Headers | show |
Comments
Does a follow-up that replaces util.deque with collections.deque make sense? I suppose the hack was the only reason for the alias... On Sat, May 16, 2015, 01:52 Adrian Buehlmann <adrian@cadifra.com> wrote: > # HG changeset patch > # User Adrian Buehlmann <adrian@cadifra.com> > # Date 1431759801 -7200 > # Node ID 3d14c1217117795e3b792369224ed7fa68e31123 > # Parent 1ef96a3b8b89a896f2c9f3f977dbef8f45bb0e26 > util: kill Python 2.4 deque.remove hack > > diff --git a/mercurial/util.py b/mercurial/util.py > --- a/mercurial/util.py > +++ b/mercurial/util.py > @@ -334,17 +334,7 @@ > > return f > > -try: > - collections.deque.remove > - deque = collections.deque > -except AttributeError: > - # python 2.4 lacks deque.remove > - class deque(collections.deque): > - def remove(self, val): > - for i, v in enumerate(self): > - if v == val: > - del self[i] > - break > +deque = collections.deque > > class sortdict(dict): > '''a simple sorted dictionary''' > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel >
On 2015-05-16 11:06, Martin von Zweigbergk wrote: > Does a follow-up that replaces util.deque with collections.deque make > sense? I suppose the hack was the only reason for the alias... Feel free to do that.
On Sat, May 16, 2015 at 2:11 AM Adrian Buehlmann <adrian@cadifra.com> wrote: > On 2015-05-16 11:06, Martin von Zweigbergk wrote: > > Does a follow-up that replaces util.deque with collections.deque make > > sense? I suppose the hack was the only reason for the alias... > > Feel free to do that. > Thanks, will do.
I've pushed this to the clowncopter, thanks. Will send my follow-up patch once this patch is in Matt's repo. On Sat, May 16, 2015 at 1:52 AM Adrian Buehlmann <adrian@cadifra.com> wrote: > # HG changeset patch > # User Adrian Buehlmann <adrian@cadifra.com> > # Date 1431759801 -7200 > # Node ID 3d14c1217117795e3b792369224ed7fa68e31123 > # Parent 1ef96a3b8b89a896f2c9f3f977dbef8f45bb0e26 > util: kill Python 2.4 deque.remove hack > > diff --git a/mercurial/util.py b/mercurial/util.py > --- a/mercurial/util.py > +++ b/mercurial/util.py > @@ -334,17 +334,7 @@ > > return f > > -try: > - collections.deque.remove > - deque = collections.deque > -except AttributeError: > - # python 2.4 lacks deque.remove > - class deque(collections.deque): > - def remove(self, val): > - for i, v in enumerate(self): > - if v == val: > - del self[i] > - break > +deque = collections.deque > > class sortdict(dict): > '''a simple sorted dictionary''' > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel >
On May 16, 2015, at 2:33 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> I've pushed this to the clowncopter, thanks. Will send my follow-up patch once this patch is in Matt's repo.
If you want, I’d be happy to review the removal of the name forward series and then you could clowncopterize it.
On 05/16/2015 02:06 AM, Martin von Zweigbergk wrote: > Does a follow-up that replaces util.deque with collections.deque make > sense? I suppose the hack was the only reason for the alias... I feel we should be methodical about this. Dropping the check/util implementation without actually cleaning the code base is a free ticket to obscure technical debt.
On 2015-05-19 07:56, Pierre-Yves David wrote: > > > On 05/16/2015 02:06 AM, Martin von Zweigbergk wrote: >> Does a follow-up that replaces util.deque with collections.deque make >> sense? I suppose the hack was the only reason for the alias... > > I feel we should be methodical about this. Dropping the check/util > implementation without actually cleaning the code base is a free ticket > to obscure technical debt. > I also feel you should be methodical about this. Separating the action in two patches was actually a good thing. BTW, the code is actually quite sprinkled with technical debt already. Cleaning it up incrementally is better than not making any progress at all. BTW 2, there may have actually been good reasons for keeping the deque alias a bit. After all, it was public. I'm perfectly ok with not sending such patches any more though, if you don't want them.
On 2015-05-17 01:08, Martin von Zweigbergk wrote: > What is it that makes it only "good enough"? Is the imports you are not > quite happy with? No. I'm fine with the imports. You even seem to have been removing one which is no longer needed after removing a use of the removed alias. I think I wrote "good enough" because I'm not able to verify the patch completely without redoing the work you did. This is one of the sort of patches where you can't say it is obviously correct or not. If you've run the testsuite, then it is ok. I didn't even run the testsuite myself, because I think I can trust you having done that. (BTW, we seem to spend an amazing amount of time on such minor issues.)
Patch
diff --git a/mercurial/util.py b/mercurial/util.py --- a/mercurial/util.py +++ b/mercurial/util.py @@ -334,17 +334,7 @@ return f -try: - collections.deque.remove - deque = collections.deque -except AttributeError: - # python 2.4 lacks deque.remove - class deque(collections.deque): - def remove(self, val): - for i, v in enumerate(self): - if v == val: - del self[i] - break +deque = collections.deque class sortdict(dict): '''a simple sorted dictionary'''