Page MenuHomePhabricator

states: created new class to handle multi-step unfinished states
AbandonedPublic

Authored by taapas1128 on Jun 7 2019, 3:42 AM.

Details

Reviewers
martinvonz
durin42
Group Reviewers
hg-reviewers
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 5 basic arguments which include the name of the command, the
name of the state file associated with it , clearable flag , allowcommit flag and
stopflag which determines whether the command supports --stop option or not.

It further contains a unified way off returning status and hint messages.
The functions of the class generate these messages based on the name of the
command and stopflag.

Merge has been handles seperately with the help of predicate instead of a
filename.

This imports unfinishedstates, checkunfinished(), and clearunfinished()
from cmdutil.py and makes it compatible with the new statecheck class.
This also adds support of the new class state.statecheck
throughout core and extensions. This also removes the
previous method cmdutil.unfinishedstates and its API.

The results are shown in tests.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Jun 7 2019, 3:42 AM
pulkit added a subscriber: pulkit.Jun 7 2019, 7:40 AM

We already have mercurial/state.py and there is a cmdstate class. We can extend that class to have these variables, but for now let's move this class to that file instead of creating a new one.

mercurial/statecheck.py
10 ↗(On Diff #15359)

Nice, looks good generally. Let's add another function to class isunfinished() which will tell whether this operation is unfinished or not.

22 ↗(On Diff #15359)

nit: unrequired new lines between in help

48 ↗(On Diff #15359)

nit: remove new lines between if and else if and else

64 ↗(On Diff #15359)

nit: return _('') .. instead of creating a temp variable.

69 ↗(On Diff #15359)

once we have a isunfinished function, this can be part of that under if cmd == 'merge'

pulkit added inline comments.Jun 7 2019, 7:45 AM
mercurial/statecheck.py
40 ↗(On Diff #15359)

We generally prepend _ to variable names if they are private to that class. That is, we don't want them to be accessed outside of the class. This is just a coding guidelines which is quite usually followed. It will be nice, if we don't add _ before variables which can be used outside the class. Like clearable maybe.

42 ↗(On Diff #15359)

nit: def hint(..)

61 ↗(On Diff #15359)

nit: def msg

This class seems unused. The steps I proposed were intended to give a slow transition, while having the code used at every step. This patch is *harder* for me to review than the original large patch was, because now I have to go and look at the other patches to see if this class behaves correctly. Can you follow the steps I suggested instead? Otherwise I'd prefer to just resurrect the old patch even though it's much larger than I'd like.

taapas1128 marked 5 inline comments as done.Jun 7 2019, 9:42 AM
taapas1128 updated this revision to Diff 15362.
taapas1128 marked 3 inline comments as done and an inline comment as not done.Jun 7 2019, 9:49 AM

@martinvonz okay I will do that then send the whole code divided as patches.

taapas1128 retitled this revision from states: created new class to handle unified states(gsoc-19) to states: created new class to handle multi-step operation states.Jun 7 2019, 10:25 AM
taapas1128 updated this revision to Diff 15367.
martinvonz requested changes to this revision.Jun 7 2019, 10:55 AM

I'll mark this as "Request Changes". That way it's clearer that it's not ready for review. When you're done, you can set the status back to "Ready for Review" (or whatever the status is called).

This revision now requires changes to proceed.Jun 7 2019, 10:55 AM
taapas1128 requested review of this revision.Jun 7 2019, 12:11 PM

@martinvonz I have updated the stack(D6487, D6488, D6494) for review up to incorporation of the checkunfinisheds() and clearunfinished() in the new API. Tests are in 3rd commit D6494. have a look whenever you are free.

martinvonz requested changes to this revision.Jun 7 2019, 12:31 PM

This still looks unused. Can you see if you can split up according to the steps I suggested, or tell me why that won't work or why you don't think it's a good idea?

This revision now requires changes to proceed.Jun 7 2019, 12:31 PM

This still looks unused. Can you see if you can split up according to the steps I suggested, or tell me why that won't work or why you don't think it's a good idea?

Oh, and I'm on vacation for a week now. It feels like we should VC about this, but that's not possible. I think Pulkit understood what I suggested, so maybe he can help explain in more detail of necessary?

taapas1128 requested review of this revision.Jun 7 2019, 3:52 PM

D6487 : A new statecheck class is created with its utility functions.
D6488 : A new list of statecheck objects is created and checkunfinished and clearunfinished are imported and modified in accordance to the class and the new list
D6494 : cmdutil.checkunfinished and cmdutil.clearunfinished is removed . Throughout core and extensions they are replaced by statemod.checkunfinished and statemod.clearunfinished. Also, the previous appends in extensions that were made to cmdutil.unfinishedstates are now made to statemod.unfinishedstates as statecheck objects. Tests results are stated
D6495 : Support for the functionalities is added to`cmdutil.STATES` is added to statemod.unfinishedstates. So cmdutil.STATES is completely removed . Tests results are stated.

@martinvonz @pulkit : Have a look now the stack is complete.

martinvonz requested changes to this revision.Jun 8 2019, 10:33 AM

Can you squash the next two patches into this one? It's hard to review it in its current form, because you need to go back and forth between the first three patches to understand how the code was moved.

This revision now requires changes to proceed.Jun 8 2019, 10:33 AM
taapas1128 requested review of this revision.Jun 8 2019, 11:25 AM

@martinvonz check it out I haveve done that.

martinvonz requested changes to this revision.Jun 8 2019, 11:29 AM

@martinvonz check it out I haveve done that.

Not according to Phabricator. Maybe you need to update the state in Phabricator?

This revision now requires changes to proceed.Jun 8 2019, 11:29 AM
taapas1128 requested review of this revision.Jun 8 2019, 11:43 AM

Regarding the message change that was previously in D6494 :

Doing that would be kind of difficult because these changes are not due to refactoring but due to statecheck.hint() which is a unified function for generating hint messages. If I change the messages in accordance to cmdutil.unfinishedstates then this problem will be there in the in integration of STATES. If you still want it I can do that by removing statecheck.hint() completely and getting the messages from cmdutil.py. So should I do that ?

martinvonz requested changes to this revision.Jun 8 2019, 12:03 PM

@martinvonz Is it alright now?

No, this patch still appears to add a new class without updating callers. I can't tell if the new code does the same thing as the old code without looking at later patches.

This revision now requires changes to proceed.Jun 8 2019, 12:03 PM

So should I squash the D6487 and D6488 too? That would make this patch really long as the very first patch that I sent?

taapas1128 retitled this revision from states: created new class to handle multi-step operation states to states: created new class to handle multi-step unfinished states.Jun 8 2019, 12:30 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15390.

So should I squash the D6487 and D6488 too?

Yes, please do.

That would make this patch really long as the very first patch that I sent?

Well, you could split them up as I suggested earlier :)

I have squashed them now . Splitting them into exactly those steps would not have been possible that is why is split them this way. Is this alright for your reviewing or should I try splitting them again? :(

taapas1128 abandoned this revision.Jun 8 2019, 5:00 PM

Splitting them into exactly those steps would not have been possible

Why not? The current form is better than before, but I would like to know why my suggestion is not possible. My suggested step 1 is surely possible.

mercurial/cmdutil.py
3301

Since bisect was intentionally excluded before this patch, it's probably best to add it in a separate patch. That way you get a place (i.e. the commit message) to explain why it was excluded before and why it should no longer be excluded.

mercurial/state.py
104

Why?

Please ignore my latest messages. I had not noticed yet that this was abandoned.