feat: promisify app.getFileIcon()#15742
Conversation
fa5b9d4 to
06ce20a
Compare
ckerr
left a comment
There was a problem hiding this comment.
The pieces that are here LGTM. Nice work!
Before we start merging PRs though I think we need a plan to make the API churn more managable to Electron app developers, e.g. the framework discussed at https://github.com/electron/maintainers/issues/159 (CC @alexeykuzmin, @MarshallOfSound).
Additionally, whatever plan we follow should be in a public repo. E.g. if we still like 159 we should move it verbatim into electron/electron/issues
Marking as Request changes to press pause while getting feedback from pineapples on how we want this rollout to proceed not due to any code issues in the PR.
|
Some thoughts: The initial plan for promisification hinged on waiting until Node 8 (which bundled However, this means that we can't, then, batch convert over all relevant methods at once, and need to do it in this more piecemeal manner. My proposal would be that we maintain a public project board with our progress on this issue, in addition to an issue in maintainers, and merge these piecemeal rather than bottleneck on each method being converted so that we can flag them all at once. What are y'all's thoughts? ^convo continued in another issue |
|
dangit meant to comment not close and comment |
06ce20a to
88e2b36
Compare
bfc7898 to
c0f1dfe
Compare
deepak1556
left a comment
There was a problem hiding this comment.
LGTM with minor change. Thanks!
|
Release Notes Persisted
|
Description of Change
Promisifies
app.getFileIconnatively.Todo:
/cc @ckerr @deepak1556
Checklist
npm testpassesRelease Notes
Notes: Promisifies
app.getFileIconCo-authored-by: Charles Kerr ckerr@github.com