Page MenuHomePhabricator

overlayworkingctx: rename misleadingly named `isempty()` method
ClosedPublic

Authored by mjacob on Fri, Jul 10, 7:16 PM.

Details

Summary

This method is only about whether there are file changes, not about whether the
commit will be empty or not.

One user of the method was incorrectly assuming the latter meaning, leading to
the bug for which a test case was added in D8727. I’ve added a FIXME to the
code.

The original motivation for the rename was that I want to add
committablectx.isempty(), that properly checks if a commit will be empty,
using the exact same logic as in repo.commit(), and I wanted to avoid a name
clash.

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

mjacob created this revision.Fri, Jul 10, 7:16 PM
mjacob updated this revision to Diff 21862.Sat, Jul 11, 5:11 PM
martinvonz added inline comments.Mon, Jul 13, 8:57 AM
mercurial/context.py
2519–2529

I feel like it would be better to find better names for these three methods together since they're about the same concept. Maybe simply renaming this one to isclean() matches well? The only issue I see is that Mercurial usually skips the "is" prefix and "clean" is both a verb and an adjective, so it's not obvious at first why there's both clean() and isclean() (people familiar with Mercurial naming might think they're the same). Probably still okay?

mjacob added inline comments.Mon, Jul 13, 11:19 AM
mercurial/context.py
2519–2529

To be honest, I didn’t think too hard about this name (at long as it’s more correct as the old name), as the method will become unused and removed a few patches later. If you still think we should rename it for the in-between steps, I’d be fine renaming it.

martinvonz added inline comments.Mon, Jul 13, 4:47 PM
mercurial/context.py
2519–2529

No, that's fine. I had just not realized that it was going away in a later patch.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.