D6501: state: created new class statecheck to handle unfinishedstates

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in state.py: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__()

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?

> state.py:91
>  
> -# A list of state files kept by multistep operations like graft.
> -# Since graft cannot be aborted, it is considered 'clearable' by update.
> -# note: bisect is intentionally excluded
> -# (state file, clearable, allowcommit, error, hint)
> -unfinishedstates = [
> -    ('graftstate', True, False, _('graft in progress'),
> -     _("use 'hg graft --continue' or 'hg graft --stop' to stop")),
> -    ('updatestate', True, False, _('last update was interrupted'),
> -     _("use 'hg update' to get a consistent checkout"))
> -    ]
> +class statecheck(object):
> +    """a utility class that deals with multistep operations like graft,

mark this "private" by prepending name with `_`?

> state.py:102
> +                 cmdmsg="", cmdhint=""):
> +        """cmdname is the name the command
> +        fname is the file name in which data should be stored in .hg directory.

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

> state.py:110
> +        state or not.
> +        cmdmsg is used to pass a different status message in case standard
> +        message of the format "abort: cmdname in progress" is not required.

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

> state.py:111
> +        cmdmsg is used to pass a different status message in case standard
> +        message of the format "abort: cmdname in progress" is not required.
> +        cmdhint is used to pass a different hint message in case standard

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

> state.py:141
> +# A list of statecheck objects for multistep operations like graft.
> +unfinishedstates = []
> +

This can also be "private", I think

> state.py:144-145
> +def addunfinished(cmddata):
> +    """this accepts a dict having command data, makes a statecheck object and
> +    adds it to the unfinishedstate object list
> +    """

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6501/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6501

To: taapas1128, durin42, martinvonz, #hg-reviewers
Cc: pulkit, mjpieters, mercurial-devel