Page MenuHomePhabricator

Rename `DifferentialAction::ACTION_REJECT` to `DifferentialAction::ACTION_CHANGE`.
AbandonedPublic

Authored by joshuaspence on May 27 2014, 9:07 AM.
Tags
None
Referenced Files
F14377864: D9303.id.diff
Sat, Dec 21, 1:48 AM
Unknown Object (File)
Thu, Dec 19, 6:22 PM
Unknown Object (File)
Sat, Dec 14, 7:16 AM
Unknown Object (File)
Wed, Dec 11, 10:57 AM
Unknown Object (File)
Wed, Dec 11, 3:03 AM
Unknown Object (File)
Sun, Dec 8, 9:16 AM
Unknown Object (File)
Thu, Dec 5, 10:07 AM
Unknown Object (File)
Thu, Dec 5, 8:00 AM
Subscribers

Details

Summary

Ref T4720. DifferentialAction::ACTION_REJECT is more suited for the "permanently reject this revision" action.

Test Plan

I haven't tested this yet. Will update once I get a dev box up and running.

Diff Detail

Repository
rP Phabricator
Branch
diff-change
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 717
Build 717: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Rename `DifferentialAction::ACTION_REJECT` to `DifferentialAction::ACTION_CHANGE`..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

It may not be worth making these changes just for the sake of having "nicer" constants. If it's not worth the effort then we can just leave as is.

epriestley edited edge metadata.

Yeah, I think we shouldn't pursue this, at least for now:

  • Installs with large datasets are pretty sensitive to the downtime caused by migrations of large tables; this is a large-ish table and the migration is low-value.
  • differential_comment no longer has readers or writers and is just a safety table in case we botched the migration. D8242 will drop it. We can probably land that soon, but I've been waiting for all the large installs to finish the migration. The new table is differential_transaction, and the action constant is stored in newValue for transactions with transactionType equal to differential:action, so the correct form of this a bit more complicated and operates on a slightly larger dataset. Doing this change also makes un-botching any migration issues discovered later more difficult. While we're pretty much in the clear on this, we're not 100% in the clear yet. If we did want to pursue this, I'd at least want to wait until after D8242 lands.
  • I think ACTION_CHANGE, while probably more correct, is also a bit more ambiguous than ACTION_REJECT. Without context, ACTION_CHANGE could mean almost anything.
  • This constant has been around for a very long time and it's relatively likely (compared to other janitorial/cleanup changes we might make) that installs (particularly Facebook) may be using it in third-party code.
  • We don't have a compelling feature-related reason to do this. This is more of a public relations / communication issue, but it's vaguely nice to couple compatibility breaks with significant feature changes since it's less of a pain for users to deal with breaks if they feel like they're getting something cool out of it.
  • To the degree that this is moving the current REJECT action out of the way to make room for a new REJECT action, I think that's not desirable since it would silently change the behavior of third-party code. It's OK to break handlers and such occasionally, but clean breaks where you hit an undefined runtime constant are a lot better than silent breaks where the behavior changes without notice.

Also:

  • I think the T4720 change should just be an "ABANDON" action under the hood (not a new type of action); we'll just permit non-authors to perform it. This will be a little bit confusing since the UI text will probably be "reject", but it won't be as bad as a new REJECT action or state existing.
This revision now requires changes to proceed.May 27 2014, 1:02 PM

Yeah, I agree with you on this and had the feeling that this was a bad idea. I am happy to just modify the behavior of DifferentialAction::ACTION_ABANDON rather than create a new state entirely.