This is an archive of the discontinued Mercurial Phabricator instance.

state: created new class statecheck to handle unfinishedstates
ClosedPublic

Authored by taapas1128 on Jun 8 2019, 4:57 PM.
Tags
None
Subscribers

Details

Summary

For the purpose of handling states for various multistep operations like
hg graft, hg histedit, hg bisect et al a new class called statecheck
is created .This will help in having a unified approach towards these commands
and handle them with ease.

The class takes in 4 basic arguments which include the name of the command, the
name of the state file associated with it , clearable flag , allowcommit flag.

This also also adds the support of`checkunfinished()` and
clearunfinished() to the class.

Tests remain unchanged.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

taapas1128 created this revision.Jun 8 2019, 4:57 PM
martinvonz requested changes to this revision.Jun 8 2019, 6:09 PM
martinvonz added inline comments.
hgext/histedit.py
2316

We may eventually want to add a function for registering states and thereby be able to make unfinishedstates and statecheck private. This is good for now, though.

mercurial/state.py
103

Why? (I asked the same thing on a different patch, but I think that one may have been dropped. I'm on mobile and too lazy to check.)

105

It looks like the code actually just deletes the state file (fname). Should we say that here instead? (I'm not sure we should.)

118

It seems unfortunate to have to write each message in this function. That's going to make it a little harder for extensions to add their commands to the list. Perhaps the hint should instead be passed in to the statecheck constructor (much like it was before this patch)? Same thing with msg() below.

143

Should this be left for a later patch? I don't see "merge" getting registered anywhere yet.

This revision now requires changes to proceed.Jun 8 2019, 6:09 PM
taapas1128 marked an inline comment as done.Jun 9 2019, 6:29 AM
taapas1128 added inline comments.
mercurial/state.py
103

actually I merge is not detected w.r.t statefile but with the help of a predicate but since it needs be part of the same class I named the statefile as None for it.

105

telling that here would be fine I suppose so I should add it here.

118

I might have a better solution to it what if I leave this as it is and add two more variables to the constructor specialmsg=None and specialhint=None. In case the extension uses any special message or hint in place of standard ones these two values can be given accordingly and standard messages can be overidden.

143

yeah it should be part of the next patch but having it here will make isfinishedcomplete

taapas1128 marked an inline comment as done.Jun 9 2019, 7:07 AM
taapas1128 updated this revision to Diff 15400.
taapas1128 marked an inline comment as done.Jun 9 2019, 7:11 AM
taapas1128 added inline comments.
mercurial/state.py
118

I have implemented that have a look. For core commands I have retained the if cases for hgext I have replaced that

143

removed that

taapas1128 updated this revision to Diff 15403.Jun 9 2019, 8:53 AM
taapas1128 updated this revision to Diff 15407.Jun 9 2019, 10:47 AM
martinvonz added inline comments.Jun 10 2019, 9:27 AM
mercurial/state.py
101

Not: we usually don't include "special" in the names of this type of argument. A default of None often implies that it's calculated in some way if it's not provided.

106

Nit: The . got lost here. Without it, hg update -C might also update to another commit.

125

Should move the graft and update cases to where they're registered (around line 152)

140

Nit: return here and in the case below for consistency (or assign to msg also on line 145) .

147

Nit: mergecheck seems unused and it seems no callers pass in a value for it, so it could probably be removed (at least for now).

pulkit added a subscriber: pulkit.Jun 10 2019, 9:38 AM
pulkit added inline comments.
mercurial/state.py
133

nit: no need of this else

151

nit: space before and after the operator

taapas1128 marked 8 inline comments as done.Jun 10 2019, 10:56 AM
taapas1128 updated this revision to Diff 15424.Jun 10 2019, 1:54 PM
pulkit added inline comments.Jun 10 2019, 7:01 PM
hgext/histedit.py
2316

the code around these statements doing append is looking a bit complex. for making it cleaner and more readable, let's do either one of thing:

  1. initialize an object before and then append
  2. improve formatting of this

This applies to all the append which are there in the patch.

mercurial/state.py
91

this comment on top of unfinished state is lost. We should reword and keep it.

92

that will to deal -> that deals

116

prepend all the class names with an _, like self._cmdname, self._fname etc. only the variables

That's a code style to say they are internal to readers and class users.

140

needs space between = operator. I guess this is caught by test-check*, make sure you run tests.

taapas1128 added inline comments.Jun 11 2019, 4:25 AM
mercurial/state.py
140

oh I will correct that. Strange it wasn't caught in test-check-code.t. I always try to run that.

taapas1128 added inline comments.Jun 11 2019, 4:28 AM
mercurial/state.py
140

I tried again it was not detected. Should I file this as a bug ?

taapas1128 marked 5 inline comments as done.Jun 11 2019, 9:47 AM
taapas1128 updated this revision to Diff 15438.
taapas1128 updated this revision to Diff 15514.Jun 14 2019, 3:28 PM
taapas1128 edited the summary of this revision. (Show Details)Jun 14 2019, 3:47 PM
pulkit added inline comments.Jun 14 2019, 6:48 PM
mercurial/state.py
141

add a new line after this.

142

we can accept cmddata here as dict, no need for **. They can be removed from callers of it's function too. Only use when we pass that to statecheck.init()

taapas1128 edited the summary of this revision. (Show Details)Jun 15 2019, 6:39 AM
taapas1128 updated this revision to Diff 15527.
taapas1128 marked 2 inline comments as done.Jun 15 2019, 6:41 AM
taapas1128 edited the summary of this revision. (Show Details)
martinvonz added inline comments.Jun 16 2019, 4:53 PM
mercurial/state.py
91–157

mark this "private" by prepending name with _?

102

Perhaps this should be opname instead? It's not always a command.

110

I'm not sure the cmd prefix in cmdmsg and cmdhint adds anything

111

nit: I think you mean s/required/desired/

141

This can also be "private", I think

142

I think it would be simpler to make addunfinished() take four arguments instead of a dict. That's what we usually do. @pulkit, what do you think? Is there are reason to pass a single dict here?

144–145

It's more important to document what it does than how it does it. The callers don't need to know about the stuff that's currently documented here.

taapas1128 added inline comments.Jun 17 2019, 3:38 AM
mercurial/state.py
142

there are not just 4 arguments but arguments for personazed messages too. Also this method has been made simin=lar to the registration method of registrar which also takes a dict.

taapas1128 added inline comments.Jun 17 2019, 3:41 AM
mercurial/state.py
110

I did that to keep a different name as compared to default messages.

taapas1128 marked 5 inline comments as done.Jun 17 2019, 7:36 AM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15535.
martinvonz added inline comments.Jun 17 2019, 11:47 AM
mercurial/state.py
142

there are not just 4 arguments but arguments for personazed messages too.

merge.update() has 11 arguments :P That doesn't mean it's good, though, and that example is actually pretty bad. I think 6-7 arguments isn't bad, and using a dict is especially bad because it loses type safety (you can do data = {'claerable': True} and not notice that you misspelled it). It might be okay to create a simple type for this, but I think it's going to be simpler to just use separate function arguments.

Also this method has been made simin=lar to the registration method of registrar which also takes a dict.

Where is that? I looked a bit in registrar.py but I couldn't find it.

I was talking about the way in which registrar takes in data. eg: see line 168 of hgweb_mod.py. Also wherever registrar is imported see data structure for cmdtable. If you still want to break it up into an argument based function I can do that too.

I was talking about the way in which registrar takes in data. eg: see line 168 of hgweb_mod.py.

I'm pretty sure that registrar.templatekeyword() doesn't know about those dict items; it really is just a generic dict there.

Also wherever registrar is imported see data structure for cmdtable.

I don't follow this example.

If you still want to break it up into an argument based function I can do that too.

Yes, please do.

taapas1128 updated this revision to Diff 15551.Jun 17 2019, 2:08 PM
pulkit added inline comments.Jun 17 2019, 4:09 PM
mercurial/state.py
142

I think it would be simpler to make addunfinished() take four arguments instead of a dict. That's what we usually do. @pulkit, what do you
think? Is there are reason to pass a single dict here?

I agree that passing four arguments will be better than a dict. I didn't realize that before reading your latest comment.

av6 added a subscriber: av6.Jun 18 2019, 2:38 AM
av6 added inline comments.
mercurial/state.py
151

It's funny how Pulkit's comment is now at this line and looks like asking to add spaces around = here. But in this case = is not an operator, it's used to indicate keyword arguments. We don't want spaces around it (similarly to PEP8).

Bad:

addunfinished(
    'graft',
    fname = 'graftstate',
    clearable = True,
    allowcommit = False,
    cmdhint = _("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

Good:

addunfinished(
    'graft',
    fname='graftstate',
    clearable=True,
    allowcommit=False,
    cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

Even better (as long as it fits into 79 characters per line):

addunfinished(
    'graft', 'graftstate', clearable=True, allowcommit=False,
    cmdhint=_("use 'hg graft --continue' or 'hg graft --stop' to stop")
)

There's a lot of places where this needs to be fixed in this series, not just here.

martinvonz added inline comments.Jun 18 2019, 2:57 AM
hgext/shelve.py
1148

A good patch to send right after this patch is one that removes this line and lets shelve use the default message. I understand why they picked "already" (because it makes sense when you run hg shelve/unshelve with an ongoing unshelve), but it doesn't make sense when e.g. running hg commit. There is already a test showing the bad example (and the good one) in test-shelve.t.

mercurial/state.py
100

I'm not sure if we should have a default for clearable. Not having a default would force callers to think about it, which may be good. OTOH, the default of False is pretty safe, so it's probably fine to have a default. But if we do have a default, I think you should update the callers to not pass clearable when they want the default.

153

Don't specify allowcommit when it's the same as the default (the same applies in other places)

taapas1128 updated this revision to Diff 15564.Jun 18 2019, 3:45 AM
taapas1128 marked 4 inline comments as done.Jun 18 2019, 3:49 AM
taapas1128 added inline comments.
mercurial/state.py
151

Thank you I will take care from now on :)

pulkit added inline comments.Jun 18 2019, 7:16 AM
hgext/phabricator.py
53 ↗(On Diff #15564)

unrelated change?

hgext/rebase.py
1953

need a space after ,.

hgext/strip.py
11 ↗(On Diff #15564)

unrelated change?

mercurial/state.py
94

s/of/or (unfinished condition or not)

taapas1128 marked 2 inline comments as done.Jun 18 2019, 8:00 AM
taapas1128 added inline comments.
hgext/phabricator.py
53 ↗(On Diff #15564)

need to amend it D6484. sorry.

taapas1128 marked 2 inline comments as done.Jun 18 2019, 8:22 AM
taapas1128 updated this revision to Diff 15570.
martinvonz accepted this revision.Jun 18 2019, 11:27 AM
This revision is now accepted and ready to land.Jun 18 2019, 11:27 AM

At some point in this series (probably as a separate patch at the end), you should update relnotes/next to describe the API change and how to register stateful operations

martinvonz added inline comments.Jun 20 2019, 2:24 AM
mercurial/state.py
154

Add space before fname

taapas1128 updated this revision to Diff 15613.Jun 20 2019, 4:03 AM
taapas1128 marked an inline comment as done.Jun 20 2019, 4:18 AM

@martinvonz relnotes/next have been updated in D6557.

taapas1128 updated this revision to Diff 15639.Jun 24 2019, 5:33 AM