Fixes T4418. Allows Maniphests created through the arc todo workflow to have projects assigned.
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
- Repository
- rARC Arcanist
- Branch
- todo-project
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1356 Build 1356: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
src/workflow/ArcanistTodoWorkflow.php | ||
---|---|---|
89–93 | 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. |
Perhaps ConduitAPI_project_query_Method should raise an error if a specified slug doesn't exist?
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 | ||
---|---|---|
98 | 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)). |
I think we should treat the slug rules as a black box -- they're relatively complicated:
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":[]}
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?
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?
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.
I think this should use slugMap so it continues working after we start normalizing slugs?