Page MenuHomePhabricator

annotate: track whether a particular annotation was the result of a skip
ClosedPublic

Authored by sid0 on Oct 2 2017, 5:35 AM.

Details

Summary

We're going to expose this information in the UI in an upcoming patch.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sid0 created this revision.Oct 2 2017, 5:35 AM

Before I stamp this I'd like an answer to the mutability concerns.

mercurial/context.py
1164–1165

I see that we're copying a ref to the object instead of making an object copy. When we had tuples, that was fine because tuples are immutable. But with attr, instances can be modified.

Will this pose any problems?

1177

Ditto.

sid0 added inline comments.Oct 2 2017, 10:56 AM
mercurial/context.py
1164–1165

Good question! Not in this case, because a particular annotation can never go from skip=True to skip=False. If we decide to overwrite the annotation afterwards, the whole object is replaced, not just fctx and lineno.

sid0 added a comment.Oct 2 2017, 10:57 AM

I'll add a comment explaining that.

sid0 added inline comments.Oct 2 2017, 10:59 AM
mercurial/context.py
1164–1165

Actually -- hmm, you're right, this is a bit risky to code changes in the future -- especially if the same object gets shared between skipped and not-skipped lines. I'll create a new one to be safe.

sid0 updated this revision to Diff 2329.Oct 2 2017, 11:05 AM
sid0 updated this revision to Diff 2331.Oct 2 2017, 11:08 AM
sid0 added a comment.Oct 2 2017, 11:09 AM

Turns out it actually was buggy. Thanks for catching it!

I switched to using attrs.evolve (and in the earlier diff setting frozen=True to make the attr immutable)

indygreg accepted this revision.Oct 2 2017, 1:14 PM

Immutability is a magical property, isn't it ;)

This revision is now accepted and ready to land.Oct 2 2017, 1:14 PM
This revision was automatically updated to reflect the committed changes.