Page MenuHomePhabricator

states: moved cmdutil.unfinishedstates to state.py
ClosedPublic

Authored by taapas1128 on Jun 5 2019, 11:20 PM.

Details

Summary

This moves cmdutil.unfinishedstates, checkunfinished(),clearunfinished()
to state.py. the already existing users of this module are updated accordingly.

Test results 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 5 2019, 11:20 PM
taapas1128 edited the summary of this revision. (Show Details)Jun 5 2019, 11:26 PM
martinvonz requested changes to this revision.Jun 6 2019, 12:18 AM

Thanks for working on this!

I think this patch does too much at once. At least the help message change should be moved to a separate patch. Ideally, the other changes would also be split up. Perhaps something like this:

  1. Move unfinishedstates, checkunfinished, and clearunfinished (without changing them) to the new module.
  2. Create and use the statecheck class, but only let it handle what's needed for unfinishedstates so far.
  3. Add the functionality from STATES to the new module and update the already existing users, but leave the cases that are handled by STATES unchanged for now.
  4. Switch existing users of STATES over to the new module (and delete STATES)
  5. Change messages to the new "To continue: ..." form. I'm not sure we want that change, or we'll probably at least want to make some, so this one is extra important to keep separate.

You don't have to follow those steps exactly, of course. You know the code better, so maybe some of what I suggested doesn't make sense.

hgext/rebase.py
1956

Pass at least the boolean arguments as named arguments (clearable=False etc)

This revision now requires changes to proceed.Jun 6 2019, 12:18 AM

@martinvonz I have completed steps 1 to 5 as you stated in the sequence above in statecheck.py. For the purpose of showing that it works without flaw, I redirected the API calls to the new API for both STATES and unfinished state and removed those from cmdutil.py. All that remains is a minor bug that I am facing with hg bisect. I will clear that soon enough. Do you want me to send the API integration and tests as a separate patch and API as another?

pulkit added a subscriber: pulkit.Jun 6 2019, 7:56 AM

@martinvonz I have completed steps 1 to 5 as you stated in the sequence above in statecheck.py. For the purpose of showing that it works without flaw, I redirected the API calls to the new API for both STATES and unfinished state and removed those from cmdutil.py. All that remains is a minor bug that I am facing with hg bisect. I will clear that soon enough. Do you want me to send the API integration and tests as a separate patch and API as another?

I think @martinvonz wants to say that we should split the whole work into nice small commits. This will help us to review it better and understand the whole change much better. The 1 to 5 are like, one commit should do one of those things.

I also agree with him, what do you think?

@pulkit okay I will split this into 5 commits.

taapas1128 abandoned this revision.Jun 7 2019, 3:39 AM
taapas1128 retitled this revision from states: unified api for checking unfinished states(gsoc-19) to states: moved cmdutil.unfinishedstates to state.py.Jun 8 2019, 4:57 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15392.

@martinvonz have a look at this stack now . It is strictly according to your guidelines.

martinvonz accepted this revision.Jun 8 2019, 5:47 PM

Thanks for splitting this out! Sorry about being so particular about splitting up the patch. I think it's an important part of writing patches that others will review. Hopefully it wasn't just frustrating and you can also see some benefit with it. The benefit is mostly to reviewers and future code archeologists and not to the author, so it can feel like wasting time. Remember that more people will read the patches than the people writing them (which is just you).

This revision is now accepted and ready to land.Jun 8 2019, 5:47 PM

@martinvonz At first I didn't seem to realize why the patches need to be split that way but later I realized that I wasn't looking from the perspective of a reviewer but was just getting the job done as an author. Thanks for making me work this out :)

taapas1128 updated this revision to Diff 15402.Jun 9 2019, 8:52 AM
taapas1128 updated this revision to Diff 15569.Jun 18 2019, 8:21 AM
taapas1128 updated this revision to Diff 15582.Jun 18 2019, 1:05 PM
taapas1128 updated this revision to Diff 15586.Jun 18 2019, 2:41 PM
martinvonz added inline comments.Jun 23 2019, 5:36 PM
hgext/absorb.py
1015 ↗(On Diff #15586)

I wonder if we should keep the functions in cmdutil.py and let them delegate to the new ones. That way we won't have to update all the callers. I think cmdutil sounds like a more natural place for the callers to looks for this function. That's also where bailifchanged() is, so it it makes sense that these two similar functions are in the same place.

hgext/strip.py
35–49 ↗(On Diff #15586)

Delete (looks like a bad rebase)

taapas1128 updated this revision to Diff 15638.Jun 24 2019, 5:32 AM
taapas1128 marked 2 inline comments as done.Jun 24 2019, 5:35 AM
This revision was automatically updated to reflect the committed changes.