Fixes T4418. Allows Maniphests created through the arc todo workflow to have projects assigned.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4418: Make it possible to create tasks attached to a project through "arc todo"
- Commits
- rARCf4615cd86b96: Allow the `todo` workflow to add project tags.
> ./bin/arc --trace --conduit-uri='http://phabricator.joshuaspence.com' todo "Test project" --project foo --project bar libphutil loaded from '/home/joshua/workspace/github.com/phacility/libphutil/src'. arcanist loaded from '/home/joshua/workspace/github.com/phacility/arcanist/src'. Config: Reading user configuration file "/home/joshua/.arcrc"... Config: Did not find system configuration at "/etc/arcconfig". Working Copy: Reading .arcconfig from "/home/joshua/workspace/github.com/phacility/arcanist/.arcconfig". Working Copy: Path "/home/joshua/workspace/github.com/phacility/arcanist" is part of `git` working copy "/home/joshua/workspace/github.com/phacility/arcanist". Working Copy: Project root is at "/home/joshua/workspace/github.com/phacility/arcanist". Config: Did not find local configuration at "/home/joshua/workspace/github.com/phacility/arcanist/.git/arc/config". Loading phutil library from '/home/joshua/workspace/github.com/phacility/arcanist/src'... >>> [0] <conduit> conduit.connect() <bytes = 618> >>> [1] <http> http://phabricator.joshuaspence.com/api/conduit.connect <<< [1] <http> 1,050,487 us <<< [0] <conduit> 1,051,585 us >>> [2] <conduit> project.query() <bytes = 199> >>> [3] <http> http://phabricator.joshuaspence.com/api/project.query <<< [3] <http> 294,584 us <<< [2] <conduit> 294,986 us >>> [4] <conduit> maniphest.createtask() <bytes = 313> >>> [5] <http> http://phabricator.joshuaspence.com/api/maniphest.createtask <<< [5] <http> 637,693 us <<< [4] <conduit> 638,098 us Created task T6: 'Test project' at http://phabricator.joshuaspence.com/T6
Diff Detail
Diff Detail
- Repository
- rARC Arcanist
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| src/workflow/ArcanistTodoWorkflow.php | ||
|---|---|---|
| 87–91 | If the user mistypes a project (e.g., --project difusin) we should catch that and raise an error. This is a little tricky because we can't map slugs to the resulting projects perfectly, but I think we could do this:
Since a user might specify two different slugs which are both valid and hit the same project (--project FOO --project fOo), it may not be an error even if we get fewer results back. Although maybe that should be a different error ("project X was specified twice"). But this seems OK to ignore, while typos seem important to catch. | |
Comment Actions
Perhaps ConduitAPI_project_query_Method should raise an error if a specified slug doesn't exist?
Comment Actions
While I think D9619 is good, I don't think this works. Particularly, if I type --project FOO, but the project really has slug #foo, the query will normalize the slug and find the project, but then this code will incorrectly determine that the slug wasn't found because the case is different. Is that accurate?
Elsewhere, we return a separate map (for example, the identifierMap on diffusion.querycommits). We could do this here, but it would break any existing callsites of projects.query since the top-level result format would change structure. Not the end of the world (we probably need to do this eventually to add pagination information) but seems like a lot of breaks for arc todo.
I don't think projects.query should fail on invalid slugs, either, since it's inconsistent with other methods and less useful than a method which returns partial results.
We could also return matchingSlugs on the results, but I don't particularly like the idea that the "object dictionary" we hand you had some non-object / query-dependent data. I don't think this is terrible, but it feels a bit surprising and is inconsistent with diffusion.querycommits.
| src/workflow/ArcanistTodoWorkflow.php | ||
|---|---|---|
| 96 | array_merge() in a loop has really poor runtime behavior: https://secure.phabricator.com/book/phabflavor/article/php_pitfalls/#array-merge-in-incredibl Better is: $project_slugs = array_mergev(ipull($projects, 'slugs')); Best might be: $project_slugs += array_fill_keys($project['slugs'], true); ...and then use isset() below (roughly, O(1)) instead of in_array() (roughly, O(N)). | |
Comment Actions
I think we should treat the slug rules as a black box -- they're relatively complicated:
Comment Actions
I don't quite understand the issue. The query doesn't seem to normalize the slugs.
> echo '{"slugs": ["foo"]}' | arc --conduit-uri='http://phabricator.joshuaspence.com' call-conduit project.query
Waiting for JSON parameters on stdin...
{"error":null,"errorMessage":null,"response":{"PHID-PROJ-ttomlhslujpx5sdpbu2c":{"id":"1","phid":"PHID-PROJ-ttomlhslujpx5sdpbu2c","name":"Foo","members":["PHID-USER-cb5af6p4oepy5tlgqypi"],"slugs":{"1":"foo","2":"bar"},"dateCreated":"1402422720","dateModified":"1402422728"}}}
> echo '{"slugs": ["FOO"]}' | arc --conduit-uri='http://phabricator.joshuaspence.com' call-conduit project.query
Waiting for JSON parameters on stdin...
{"error":null,"errorMessage":null,"response":[]}Comment Actions
Oh -- well, it should be normalizing -- that is, --project Diffusion, --project 'Quality Assurance', etc., should work.
One approach would be to introduce project.normalizeslugs or something?
Comment Actions
That means we'd need to do two Conduit calls... first to normalize the slugs and then to query the project. What about introducing project.queryslugs instead?
Comment Actions
We don't actually call project.query anywhere, so let's just modernize it to work like the most modern methods (e.g., diffusion.querycommits), and return:
- a list of results in data (the current results of the entire call);
- paging parameters; and
- a slugMap mapping any queried slugs to their associated projects PHIDs.
We need to do this sooner or later anyway.
Comment Actions
I think this should use slugMap so it continues working after we start normalizing slugs?