Page MenuHomePhabricator

Lint/unittest excuses overwrite each other
Closed, ResolvedPublic

Description

If you do arc diff and suppress linting (say), and you type in an excuse, the excuse shows up on the differential review page, great. If you then run arc diff again -- even arc diff --verbatim -- and suppress linting again, the new excuse overwrites the old one.

At least some people here had the expectation that the new excuse would be appended to the old excuse, rather than replacing it, due to the tendency of phabricator to do appending in other situations.

One solution would be to do appending for excuses; an equally good one (from our point of view) would just be to document the behavior better somehow. Or, perhaps, if there's an existing excuse, have arc offer that as the default when asking for a new excuse.

Event Timeline

csilvers updated the task description. (Show Details)
csilvers added a project: Differential.
csilvers added subscribers: csilvers, mroth.

The model is that the excuse is for the lint/unit state of a given diff (an individual update on a revision), so it stays attached to that diff. If you fiddle with "Revision Update History", you should be able to find any previous excuse by diffing older diffs.

In general, diff 1 may have, say, --nolint (excuse: "just sketching this out since I had a question..."), while diff 2 has lint with an error (excuse: "this looks like it's not my fault, I'm going to go ask Sarah about it"). These excuses may be excusing different things, which is why we don't prefill or append -- an earlier excuse may no longer make sense in the context of a new error state.

Does this generally make sense?

Conceivably, we could special case --nolint to carry over excuses, since it's reasonable that the diff 1 excuse for skipping lint probably applies equally well to the diff 2 excuse for skipping lint, but this feels fairly magical to me.

(On documenting the behavior, is there anywhere in particular that you looked for an explanation where we could add documentation?)

I don't know where I'd look for documentation; where the excuse was I guess.

The particular situation I was seeing was the excuse was '(same as before)' but I didn't know what 'before' was, or have a way of seeing it. It didn't occur to me to fiddle with the revision update history!

I'm not sure there's a great solution here, just wanted to put it on your radar that at least some people were confused by how this all worked.

epriestley claimed this task.

This is mooted because I've removed the "excuses" feature about a year ago -- on the balance, I think it generated more confusion and busywork than signal.