| Submitter | Siddharth Agarwal |
|---|---|
| Date | Nov. 17, 2015, 11:58 p.m. |
| Message ID | <bb1b25b554d6efe340f3.1447804721@dev666.prn1.facebook.com> |
| Download | mbox | patch |
| Permalink | /patch/11447/ |
| State | Changes Requested |
| Headers | show |
Comments
On Tue, Nov 17, 2015 at 4:01 PM Siddharth Agarwal <sid0@fb.com> wrote: > # HG changeset patch > # User Siddharth Agarwal <sid0@fb.com> > # Date 1447797080 28800 > # Tue Nov 17 13:51:20 2015 -0800 > # Node ID bb1b25b554d6efe340f3c74b58fec40cd56fd557 > # Parent a879917dec18a4621476c8af18724f5d5696c663 > mergestate: add a constructor that sets up a fresh merge state > > Eventually, we'll move the read call out of the constructor. This will: > - avoid unnecessary reads when we're going to nuke the merge state anyway > - avoid raising an exception if there's an unsupported merge record > > I chose 'fresh' for the name because I wanted to avoid anything with the > word > 'new' in it, and 'reset' is more an action performed on a merge state than > a > way to get a new merge state. > When I saw patch 4/6, I was wondering how the merge state got removed from disk. It turns out that ms.reset() does that. So this method (fresh()) *is* a method for performing an action after all, right? > > diff --git a/mercurial/merge.py b/mercurial/merge.py > --- a/mercurial/merge.py > +++ b/mercurial/merge.py > @@ -75,6 +75,14 @@ class mergestate(object): > statepathv1 = 'merge/state' > statepathv2 = 'merge/state2' > > + @staticmethod > + def fresh(repo, node=None, other=None): > + """Initialize a fresh merge state, removing any existing state on > + disk.""" > + ms = mergestate(repo) > + ms.reset(node, other) > + return ms > + > def __init__(self, repo): > self._repo = repo > self._dirty = False > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > https://selenic.com/mailman/listinfo/mercurial-devel >
On 11/17/15 16:20, Martin von Zweigbergk wrote: > When I saw patch 4/6, I was wondering how the merge state got removed > from disk. It turns out that ms.reset() does that. So this method > (fresh()) *is* a method for performing an action after all, right? Right, but something like mergestate.reset() returning a new merge state sounds really weird to my (admittedly non-native) ears.
On Tue, Nov 17, 2015 at 4:25 PM Siddharth Agarwal <sid@less-broken.com> wrote: > On 11/17/15 16:20, Martin von Zweigbergk wrote: > > When I saw patch 4/6, I was wondering how the merge state got removed > > from disk. It turns out that ms.reset() does that. So this method > > (fresh()) *is* a method for performing an action after all, right? > > Right, but something like mergestate.reset() returning a new merge state > sounds really weird to my (admittedly non-native) ears. I agree, but I'd rather have something that sounds like it does something also return something than have something that sounds like it only return something also do something. Unexpected side-effects are worse than unexpected return values. It's pretty common to have methods like list.pop() that modify the list and return something, after all. Not a perfect analogy, I know. Maybe some other name?
On 11/17/15 16:33, Martin von Zweigbergk wrote: > I agree, but I'd rather have something that sounds like it does > something also return something than have something that sounds like > it only return something also do something. Unexpected side-effects > are worse than unexpected return values. It's pretty common to have > methods like list.pop() that modify the list and return something, > after all. Not a perfect analogy, I know. Maybe some other name? OK -- did 'fresh' not convey that to you? 'new' is similar but is a massively overloaded term in programming. What about 'newreset'? Then the other one, which I'm calling 'read' right now, can be 'newread'. - Siddharth
On Tue, Nov 17, 2015 at 4:49 PM Siddharth Agarwal <sid@less-broken.com> wrote: > On 11/17/15 16:33, Martin von Zweigbergk wrote: > > I agree, but I'd rather have something that sounds like it does > > something also return something than have something that sounds like > > it only return something also do something. Unexpected side-effects > > are worse than unexpected return values. It's pretty common to have > > methods like list.pop() that modify the list and return something, > > after all. Not a perfect analogy, I know. Maybe some other name? > > OK -- did 'fresh' not convey that to you? > I think the reason it didn't convey that is because I read your explanation saying that you didn't like 'reset' because it is more like an action, so I interpreted that as saying that 'fresh' is not an action. If I had just seen the method name without a motivation, I'm not sure what I would have thought. I would probably have looked at the docstring, which does say that it removes state on disk. > > 'new' is similar but is a massively overloaded term in programming. > > What about 'newreset'? Then the other one, which I'm calling 'read' > right now, can be 'newread'. > > - Siddharth > I think I would prefer mergestate.clean(). I would not be surprised that it returns the clean state. But then again, perhaps 'fresh' would also be fine if looking at it without preconception. You pick. If you choose to keep 'fresh', I can just queue the series.
On 11/17/15 16:57, Martin von Zweigbergk wrote: > I think I would prefer mergestate.clean(). I would not be surprised > that it returns the clean state. But then again, perhaps 'fresh' would > also be fine if looking at it without preconception. You pick. If you > choose to keep 'fresh', I can just queue the series. I like clean(). If you queue up the first two I'll send a V2 of the next four.
On Tue, Nov 17, 2015 at 4:58 PM Siddharth Agarwal <sid@less-broken.com> wrote: > On 11/17/15 16:57, Martin von Zweigbergk wrote: > > I think I would prefer mergestate.clean(). I would not be surprised > > that it returns the clean state. But then again, perhaps 'fresh' would > > also be fine if looking at it without preconception. You pick. If you > > choose to keep 'fresh', I can just queue the series. > > I like clean(). If you queue up the first two I'll send a V2 of the next > four. > Deal!
Patch
diff --git a/mercurial/merge.py b/mercurial/merge.py --- a/mercurial/merge.py +++ b/mercurial/merge.py @@ -75,6 +75,14 @@ class mergestate(object): statepathv1 = 'merge/state' statepathv2 = 'merge/state2' + @staticmethod + def fresh(repo, node=None, other=None): + """Initialize a fresh merge state, removing any existing state on + disk.""" + ms = mergestate(repo) + ms.reset(node, other) + return ms + def __init__(self, repo): self._repo = repo self._dirty = False