Update shell.openExternal to promise due to electron update on atom by sadick254 · Pull Request #2625 · atom/github · GitHub
Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Update shell.openExternal to promise due to electron update on atom - #2625

Merged
smashwilson merged 2 commits into
masterfrom
update-electron-apis
Feb 12, 2021
Merged

smashwilson merged 2 commits into
masterfrom
update-electron-apis

Conversation

@sadick254

Copy link
Copy Markdown
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Description of the Change

There are some Electron APIs that changed to only return Promises (and not run callbacks passed to them), mostly as of Electron 7+. Updating to be compatible with those would be great for updating Atom's Electron version.

Screenshot or Gif

Applicable Issues

#2624

@codecov

codecov Bot commented Feb 11, 2021 •

Copy link
Copy Markdown

@sadick254
sadick254 force-pushed the update-electron-apis branch 2 times, most recently from ecf308b to d08bb7b Compare February 11, 2021 06:23

@smashwilson smashwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm CodeCov is complaining about these... they must not be covered by existing tests. I think that's probably not worth addressing here, though, because these methods don't do anything but call two other functions that we'd need to stub anyway.

Comment thread lib/controllers/issueish-list-controller.js Outdated
@smashwilson

Copy link
Copy Markdown
Contributor

Looks like we'll need to update the stubbing in some tests, too.

@sadick254
sadick254 force-pushed the update-electron-apis branch 4 times, most recently from 5e165e6 to 9678921 Compare February 12, 2021 07:09

@smashwilson smashwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these could also be .returns(Promise.resolve()). No big deal though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants