See some discussion in T5245.

Er, some discussion in D9838, rather.

The naming (ApplicationEdgeTypeThing) is mostly for consistency with the similar existing ApplicationPHIDTypeThing classes. I don't really have much of a specific justification for it. One advantage is that it avoids hard-to-parse names when an application and object have the same name, like PhabricatorProjectProjectHasObjectEdgeType or PhabricatorDashboardsDashboardHasWidgetEdgeType. In almost all callsites both of these objects are just used as XYZ::BLAHCONST, and it's maybe a little easier to see the important part if it's at the end? This is all pretty fluff, but these classes sort of feel distinct enough in their usage to me to justify the convention adjustment.

I'm not opposed to changing them if that's unpersuasive, but we should change ApplicationPHIDType classes too, and will end up with some ugly ones (PhabricatorProjectProjectPHIDType).

The method-and-constant is similarly because we do that in PHIDType. The reasoning there is roughly that it's nice to have a method so we can mpull(), but also nice to have a constant so usage is slightly more clear in common cases. Defining all the constants in a regular way is a little more greppable. In theory, we could implement the methods using reflection (there's no static:: until PHP 5.3.0), but that feels maybe-too-magical?

I'm also fine with changing this, but we should change PHIDType too if we do.

In D9839#17, @epriestley wrote:

The naming (ApplicationEdgeTypeThing) is mostly for consistency with the similar existing ApplicationPHIDTypeThing classes. I don't really have much of a specific justification for it. One advantage is that it avoids hard-to-parse names when an application and object have the same name, like PhabricatorProjectProjectHasObjectEdgeType or PhabricatorDashboardsDashboardHasWidgetEdgeType. In almost all callsites both of these objects are just used as XYZ::BLAHCONST, and it's maybe a little easier to see the important part if it's at the end? This is all pretty fluff, but these classes sort of feel distinct enough in their usage to me to justify the convention adjustment.

I'm not opposed to changing them if that's unpersuasive, but we should change ApplicationPHIDType classes too, and will end up with some ugly ones (PhabricatorProjectProjectPHIDType).

Ultimately it's up to you I guess (you maintain the code base), but my personal thoughts are that:

• ManiphestTaskHasRevisionEdgeType is preferable to ManiphestEdgeTypeTaskHasRevision, mainly because it reads nicer. It also "namespaces" better. If you ever moved to PHP 5.3 and adopted namespacing (unlikely but still a valid point I think), then ManiphestTaskHasRevisionEdgeType maps nicely to something like \Phabricator\Maniphest\TaskHasRevisionEdgeType.
• Personally, I don't think that the application needs to prefix the class name in some (all?) cases. For example, I would prefer PhabricatorDashboardHasWidgetEdgeType and PhabricatorWidgetBelongsToDashboardEdgeType to PhabricatorDashboardsDashboardHasWidgetEdgeType and PhabricatorDashboardsWidgetBelongsToDashboardEdgeType (respectively). This is probably, however, not inline with your current naming conventions.
• PHIDs are global, so PhabricatorProjectPHIDType seems unique enough to me. Again, this is probably not inline with your current naming conventions.

As I said, it's ultimately your call... but these are my thoughts. Oh, and +1 on consistency (i.e. if you were to change the naming convention then you should also change the ApplicationPHIDType classes).

In D9839#17, @epriestley wrote:

The method-and-constant is similarly because we do that in PHIDType. The reasoning there is roughly that it's nice to have a method so we can mpull(), but also nice to have a constant so usage is slightly more clear in common cases. Defining all the constants in a regular way is a little more greppable. In theory, we could implement the methods using reflection (there's no static:: until PHP 5.3.0), but that feels maybe-too-magical?

Hmm yeah. Admittedly, I didn't notice that getEdgeConstant was an instance method (i.e. non-static) and I always forgot that Phabricator supports PHP 5.2. I guess, given the circumstances, the duplication between the constant and the method is tolerable. Mayyybbeee we could rename it to EDGE_CONST though?

Reducing ProjectProject to Project feels a little bit worse (inconsistent / special cased) to me than the ugliness of the double name.

I think there's probably at least an argument somewhere for \Phabricator\Maniphest\EdgeType\TaskHasRevision as a namespace, if we ever did do namespaces. I'm not sure if that argument's very strong or good. I think it's probably mostly moot anyway -- even if we bump the minimum version up to a version with namespaces, my inclination is probably not to use them (except maybe putting everything in a global \Phabricator namespace) -- I don't think they'd really solve any problems.

An alternative would be to make all names hierarchical (DifferentialControllerRevisionView, DifferentailEditorRevision) but a lot of them lose a lot of meaning.

Anyway, I'll rename these for consistency with all other non-PHIDType classes, and we can deal with PHIDType later.

Let me see if I can use reflection to get rid of the subclass method without creating too much of a mess.

I'll take a look at this now.

(why is this still in "Needs Review"? @joshuaspence "rejected prior diff" + @btrahan "accepted" == "Needs Review"?)

Rejects are sticky. The assumption is you might be objecting to something significant and want to sign off on it before it moves forward.

If you were away or on vacation or something, I could edit the revision and remove you as a reviewer, which would move it to "Accepted" (one accept + no rejects), but also leave a more clear audit trail in case I was being sneaky about it.

(There's some discussion somewhere of introducing a "soft reject", maybe, and some discussion of making comments act like a "very soft reject", to smooth out some use cases.)

Gotcha. Thanks for clarifying.