use the loadReviewedBy function, which seems to do what we want -- returns a reviewer IFF the last thing was an accept
Details
Details
- Reviewers
epriestley - Commits
- Restricted Diffusion Commit
rP8867e50d6333: Herald - tweak accepted differential revision check slightly
i believe in the power of loadReviewedBy
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Is this in response to something? I think this is maybe-bad or at least counterintuitive. I also want to get rid of loadReviewedBy() eventually.
Comment Actions
Ah, okay, I found the IRC bit.
arc land should not close revisions if the repository is tracked. However, arc is often not able to figure out that a repository is tracked right now, because this chain has to be unbroken:
- The working copy has a .arcconfig;
- the .arcconfig has a project_id;
- the project_id's corresponding "Arcanist Project" has a repository.
Usually, step (3) is missing, because it's configured in an obscure interface and there are no hints about it. I'd like to get rid of this and let arc figure out that the repository is tracked based on the remote URI.
The quick fix here is probably to go to Repositories > Whatever Project from the web UI and set a repository.
In this specific case, if that doesn't work and we need something in the short term, maybe adding a separate "Accepted or closed revision" field is a better fix? I want to get rid of loadReiviewedBy() eventually -- it no longer really makes sense given that we can track multiple "Accept"ing users, and it doesn't follow modern query protocol.
In the long run, I want to move away from Arcanist Projects as much as possible.
Comment Actions
Oh, I'm actually probably crazy. The CommitMessage worker will always fire before the Herald worker does, and always close the revision. So this patch is more consistent with intent. We can deal with the specifics when this stuff gets sorted out eventually.
Comment Actions
Cool.
I think loadReviewedBy will have a worthy successor someday. For example, that diff you rejected and I approved and the confusion there -- I could see that being configurable and thus requiring this successor someday. Or not, but we can fix this at that time I think.
(will commit this tomorrow or later if I get back on)