Page MenuHomePhabricator

Add Phrequent workflows to Arcanist
ClosedPublic

Authored by hach-que on Oct 16 2013, 1:03 PM.

Details

Summary

Depends on D9906.

This adds arc start, arc stop and arc tracking for tracking tasks, diffs and other objects in Phrequent.

Test Plan

Tested this against a local install.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

This seems pretty cumbersome. You have to type:

arc track --start --task T123

Is there a reason you went with that instead of:

arc start T123

...as in D7326?

Generally, I think the examples in D7326 (arc start, arc stop) are a lot easier to use than this. You can implement them like this:

  1. Accept a wildcard argument, which matches the T123 part.
  2. Throw if there are zero args, or more than one arg ("specify exactly one object to start work on").
  3. Use the phid.lookup method to look up the object the user names. This will convert a string like T123 into an object PHID.
  4. Call phrequent.start on the PHID.

The --task stuff is both cumbersome and not general. Phrequent supports tracking any object type, including types introduced by third-party applications. If you use phid.lookup, all of this works for free. If you don't, you'll have to implement --paste, --revision, --commit, --mock, etc., for every object type, and third-party applications won't be accessible from the CLI.

Looks like this is this parallel work to D7326, right? Should one of these be finished?

The author of these changes is a third-party contributor, so presumably he just hasn't found the time to finish them.

We'll build support for these things in the upstream eventually even if he never picks these back up, but Phrequent is a very low priority right now.

hach-que added a reviewer: skyronic.
hach-que edited edge metadata.

Reworked according to feedback

hach-que retitled this revision from Added a time tracking workflow for phrequent to Add Phrequent workflows to Arcanist.Jul 12 2014, 6:13 PM
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que edited edge metadata.
src/workflow/ArcanistStartWorkflow.php
92–93

Because the current timestamp is the same as the time that just ended, it can't distinguish between a user time that's currently running (where end time == now) and a user time that's been ended at the current time (where end time == now as well).

We should probably fix this inside Phrequent at some point, but all of the strata and time block calculations stuff is complex and I don't want to break it.

epriestley edited edge metadata.

What do you think about arc time instead of arc tracking? Offhand, it's a little shorter and seems a little more closely related...

Couple of minor inlines. This mostly looks good.

src/workflow/ArcanistStartWorkflow.php
40–45

Is there any reason to have this?

55–56

This permits arc start T1 T2 but ignores T2.

This also permits arc start --object T1 T2 but ignores T1.

68–69

throw new ArcanistUsageException(...)

74–84

These calls could use futures, although probably not worth bothering.

92–93

Sleeping seems pretty bad. We should find another way to get around this for now so users arent' stuck watiing.

src/workflow/ArcanistStopWorkflow.php
60–65

As above on arg handling.

src/workflow/ArcanistTrackingWorkflow.php
6–8 ↗(On Diff #23798)

It would be slightly nicer to make this abstract and extend all three (start, stop, tracking) from it. This workflow isn't inherently any more core.

89–99 ↗(On Diff #23798)

Can we just fake this instead of doing the sleeps?

101–107 ↗(On Diff #23798)

Use PhutilConsoleTable

This revision now requires changes to proceed.Jul 12 2014, 6:24 PM
hach-que edited edge metadata.
  • Changes according to feedback
hach-que edited edge metadata.

ArcanistUsageException and PhutilConsoleTable

epriestley edited edge metadata.
epriestley added inline comments.
src/workflow/ArcanistPhrequentWorkflow.php
63

Given the way the API currently works, this will never print 'Stopped' and always print 'in progress' on the first item. Maybe just a "*" or something to reinforce that the top item is the active one? This is easy to tweak later.

This revision is now accepted and ready to land.Jul 17 2014, 12:16 AM
hach-que updated this revision to Diff 23891.

Closed by commit rARCf658f170808e (authored by @hach-que).