Page MenuHomePhorge

Column Edit: minor refactor for future changes
AbandonedPublic

Authored by valerio.bozzolan on Mar 24 2023, 21:03.

Details

Summary

This change does not introduce any feature.

This is just a minor refactor to simplify the
introduction of other features in the future.

The real change will be implemented here:

D25066: Workboard: Milestone Name easily editable (instead of surfing 3 pages)

In case, this change will be auto-squashed by Arcanist.

Ref T15143

Test Plan
  • See that nothing was changed logically

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25066_workboard_refactor
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 176
Build 176: arc lint + arc unit

Event Timeline

src/applications/project/controller/PhabricatorProjectColumnEditController.php
86

Note that this is the exact replacement.

97

This redirect was moved at line 111. It's the exact replacement.

121–123

Note that this is the exact replacement.

src/applications/project/controller/PhabricatorProjectColumnEditController.php
81

This line has an exact replacement.

108

This line has an exact replacement.

avivey added inline comments.
src/applications/project/controller/PhabricatorProjectColumnEditController.php
97

why?

This Diff is just a way to be nice and have the real change easier to be reviewed:

D25066: Workboard: Milestone Name easily editable (instead of surfing 3 pages)

So we have two very small diffs, easier to be reviewed.

Don't worry: if we will land D25066, this will be auto-squashed. Arc Diff does that by default.

avivey requested changes to this revision.Apr 5 2023, 09:51

This diff crosses the line to "too small".

Breaking the diff into too-small chunks makes the over-all idea harder to see and review, because it makes the actual change harder to see, and can actually hide quality issues that would be easier to see in a single diff.

Plus, ideally each diff should be able to stand on its own and have some positive value; While extracting $proxy and $is_column is arguably good - it somewhat improves readability of the code, and I would have accepted a change with only those two - extracting $saved_everything has a negative net effect - it adds another variable and increases the complexity of the code.

It makes sense to break a large diff into separate diffs if each part is still large enough and has some benefit on its own, but this isn't the case here.
Please fold this into the real revision.

This revision now requires changes to proceed.Apr 5 2023, 09:51

I can surely put everything (again) in the blender if this can help you...

But please consider that the current status allows to mark this as "OK, weird, but no regressions" and then have much more review comfort on D25066: Workboard: Milestone Name easily editable (instead of surfing 3 pages) to say the same thing there.

(I had obviously just interest to be nice and simplify your work)

But please consider that the current status allows to mark this as "OK, weird, but no regressions" and then have much more review comfort on D25066: Workboard: Milestone Name easily editable (instead of surfing 3 pages) to say the same thing there.

I disagree with this statement - I think it makes the over-all changes for T15143 harder to see.