Changeset View
Standalone View
src/applications/project/storage/PhabricatorProjectColumn.php
| Show First 20 Lines • Show All 265 Lines • ▼ Show 20 Lines | return array( | ||||||||||
| ->setDescription(pht('The project the column belongs to.')), | ->setDescription(pht('The project the column belongs to.')), | ||||||||||
| id(new PhabricatorConduitSearchFieldSpecification()) | id(new PhabricatorConduitSearchFieldSpecification()) | ||||||||||
| ->setKey('proxyPHID') | ->setKey('proxyPHID') | ||||||||||
| ->setType('phid?') | ->setType('phid?') | ||||||||||
| ->setDescription( | ->setDescription( | ||||||||||
| pht( | pht( | ||||||||||
| 'For columns that proxy another object (like a subproject or '. | 'For columns that proxy another object (like a subproject or '. | ||||||||||
| 'milestone), the PHID of the object they proxy.')), | 'milestone), the PHID of the object they proxy.')), | ||||||||||
| id(new PhabricatorConduitSearchFieldSpecification()) | |||||||||||
| ->setKey('isHidden') | |||||||||||
| ->setType('bool') | |||||||||||
0: It seems odd to return the strings "0" and "1" for the status. Unless there's a reason to do… | |||||||||||
| ->setDescription(pht('True if this column is hidden.')), | |||||||||||
| id(new PhabricatorConduitSearchFieldSpecification()) | |||||||||||
| ->setKey('isDefaultColumn') | |||||||||||
| ->setType('bool') | |||||||||||
| ->setDescription(pht('True if this is the default column.')), | |||||||||||
Not Done Inline ActionsThe existing bool fields use the language "True if ..." rather than "Whether ...". 0: The existing bool fields use the language "True if ..." rather than "Whether ...". | |||||||||||
| id(new PhabricatorConduitSearchFieldSpecification()) | |||||||||||
| ->setKey('sequence') | |||||||||||
| ->setType('int') | |||||||||||
| ->setDescription( | |||||||||||
| pht('The sequence in which this column appears on the workboard.')), | |||||||||||
| ); | ); | ||||||||||
| } | } | ||||||||||
| public function getFieldValuesForConduit() { | public function getFieldValuesForConduit() { | ||||||||||
| return array( | return array( | ||||||||||
| 'name' => $this->getDisplayName(), | 'name' => $this->getDisplayName(), | ||||||||||
| 'proxyPHID' => $this->getProxyPHID(), | 'proxyPHID' => $this->getProxyPHID(), | ||||||||||
| 'project' => $this->getProject()->getRefForConduit(), | 'project' => $this->getProject()->getRefForConduit(), | ||||||||||
| 'isHidden' => $this->isHidden(), | |||||||||||
Not Done Inline Actions
Should the key be changed to match? 0: Should the key be changed to match? | |||||||||||
Not Done Inline ActionsYes, they should :) avivey: Yes, they should :) | |||||||||||
| 'isDefaultColumn' => $this->isDefaultColumn(), | |||||||||||
| 'sequence' => (int)$this->getSequence(), | |||||||||||
Not Done Inline Actions
As already mentioned I would recommend to return an integer here since all the known business logic of that information assume that as an integer, and so the API consumers should assume that as an integer. I'm not opinionated about how, but maybe a single cast can do the job instead of doing a refactor, for now. valerio.bozzolan: As already mentioned I would recommend to return an integer here since all the known business… | |||||||||||
Not Done Inline ActionsI'm suggesting a cast to int also because there are other places related to Conduit with a cast. Example: rPef13b0e52b46: Expose repository "importing" flag via diffusion.repository.search valerio.bozzolan: I'm suggesting a cast to `int` also because there are other places related to Conduit with a… | |||||||||||
Not Done Inline ActionsThe sequence should always be an integer, so adding the cast here seems reasonable. As this sort of thing is already being done in several places, perhaps the comment isn't necessary. (Another example is (int)$this->getVersion() for phriction.content.search.) Although it is odd that these methods are returning string values in the first place. This is too deep in the bowels of Phabricator for me to understand, but I don't think PhabricatorEdgeObject#getSequence() is to blame; it might have something to do with readField in LiskDAO instead. I bailed out when I saw "Black magic" in one of the comments. 0: The sequence should always be an integer, so adding the cast here seems reasonable. As this… | |||||||||||
| ); | ); | ||||||||||
| } | } | ||||||||||
| public function getConduitSearchAttachments() { | public function getConduitSearchAttachments() { | ||||||||||
| return array(); | return array(); | ||||||||||
| } | } | ||||||||||
| public function getRefForConduit() { | public function getRefForConduit() { | ||||||||||
| ▲ Show 20 Lines • Show All 73 Lines • Show Last 20 Lines | |||||||||||
It seems odd to return the strings "0" and "1" for the status. Unless there's a reason to do this, an integer would be more reasonable. Even better would be an array containing the keys "value" and "name", like in a few other status fields.