Page MenuHomePhabricator

[native] Rename callbacks in `InputStateContainer:uploadFile` for clarity
ClosedPublic

Authored by atul on Aug 22 2022, 3:27 PM.
Tags
None
Referenced Files
F3348914: D4900.id16079.diff
Fri, Nov 22, 4:30 PM
F3348879: D4900.id15839.diff
Fri, Nov 22, 4:20 PM
F3347315: D4900.diff
Fri, Nov 22, 11:50 AM
Unknown Object (File)
Mon, Nov 18, 1:18 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Unknown Object (File)
Fri, Nov 8, 8:57 PM
Subscribers

Details

Summary

The fail(...) and finish(...) naming was confusing both when defined and at callsites.

I think switching the naming to onUpload[Finished/Failed] makes things clearer.

Test Plan

This is a noop, no functional change.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Aug 22 2022, 3:38 PM
abosh added inline comments.
native/input/input-state-container.react.js
604–624 ↗(On Diff #15839)

This is a good name change. I have a question - what do you think about making these their own functions outside of uploadFile? We can just add some parameters and factor out this code which is harder to read inside of another function.

In my opinion, uploadFile should start by uploading the file (that is, the first code should be related to that, like the try/catch block below)...the reader shouldn't have to read two functions that only get called at the tail end of uploading a file before reading code that's substantive to the beginning of the function's purpose.

Outside the scope of this diff, but worth a discussion since I notice it every time I read uploadFile.

This revision is now accepted and ready to land.Aug 22 2022, 6:19 PM
native/input/input-state-container.react.js
604–624 ↗(On Diff #15839)

This is a good name change. I have a question - what do you think about making these their own functions outside of uploadFile? We can just add some parameters and factor out this code which is harder to read inside of another function.

I agree that the way things are currently organized is weird. However, want to limit refactoring as much as possible.