D6501: state: created new class statecheck to handle unfinishedstates

pulkit (Pulkit Goyal) phabricator at mercurial-scm.org
Mon Jun 10 19:02:19 EDT 2019


pulkit added inline comments.

INLINE COMMENTS

> histedit.py:2291
>      cmdutil.summaryhooks.add('histedit', summaryhook)
> -    statemod.unfinishedstates.append(
> -        ['histedit-state', False, True, _('histedit in progress'),
> -         _("use 'hg histedit --continue' or 'hg histedit --abort'")])
> +    statemod.unfinishedstates.append(statemod.statecheck('histedit',
> +    'histedit-state', clearable=False, allowcommit=True))

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.

> 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.

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

> state.py:92
> +class statecheck(object):
> +    """a utility class that will to deal with multistep operations
> +       like graft, histedit, bisect, update etc and check whether such commands

`that will to deal` -> `that deals`

> state.py:116
> +        """
> +        self.cmdname = cmdname
> +        self.fname = fname

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.

> state.py:140
> +
> +unfinishedstates=[]
> +unfinishedstates.append(statecheck('graft', 'graftstate', clearable=True,

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

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list