The Wayback Machine - https://web.archive.org/web/20210826070119/https://github.com/atom/github/pull/2625
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Feb 12, 2021

Conversation

@sadick254
Copy link
Contributor

@sadick254 sadick254 commented Feb 11, 2021

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

@sadick254 sadick254 requested a review from smashwilson Feb 11, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 11, 2021

Codecov Report

Merging #2625 (4ab44f3) into master (044931d) will increase coverage by 0.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2625      +/-   ##
==========================================
+ Coverage   93.45%   93.46%   +0.01%     
==========================================
  Files         237      237              
  Lines       13234    13215      -19     
  Branches     1906     1900       -6     
==========================================
- Hits        12368    12352      -16     
+ Misses        866      863       -3     
Impacted Files Coverage Δ
lib/controllers/issueish-searches-controller.js 89.47% <50.00%> (+8.52%) ⬆️
lib/controllers/issueish-list-controller.js 77.77% <100.00%> (-2.23%) ⬇️
lib/controllers/remote-controller.js 100.00% <100.00%> (ø)
lib/views/actionable-review-view.js 100.00% <100.00%> (ø)
lib/views/issueish-link.js 81.25% <100.00%> (-2.09%) ⬇️
lib/atom/gutter.js 92.85% <0.00%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 044931d...4ab44f3. Read the comment docs.

@sadick254 sadick254 force-pushed the update-electron-apis branch 2 times, most recently from ecf308b to d08bb7b Feb 11, 2021
Copy link
Member

@smashwilson smashwilson left a comment

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.

lib/controllers/issueish-list-controller.js Outdated Show resolved Hide resolved
@smashwilson
Copy link
Member

@smashwilson smashwilson commented Feb 11, 2021

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 Feb 11, 2021
@sadick254 sadick254 force-pushed the update-electron-apis branch from 9678921 to 4ab44f3 Feb 12, 2021
@sadick254 sadick254 requested a review from smashwilson Feb 12, 2021
Copy link
Member

@smashwilson smashwilson left a comment

@@ -90,15 +90,15 @@ describe('IssueishListController', function() {

it('calls shell.openExternal with specified url', async function() {
const wrapper = shallow(buildApp());
sinon.stub(shell, 'openExternal').callsArg(2);
sinon.stub(shell, 'openExternal').callsFake(() => { });

This comment has been minimized.

@smashwilson

smashwilson Feb 12, 2021
Member

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

@smashwilson smashwilson merged commit 8c07644 into master Feb 12, 2021
10 checks passed
10 checks passed
@github-actions
tests (ubuntu-18.04, beta)
Details
@github-actions
tests (ubuntu-18.04, nightly)
Details
@github-actions
tests (macos-10.14, beta) tests (macos-10.14, beta)
Details
@github-actions
tests (macos-10.14, nightly) tests (macos-10.14, nightly)
Details
@github-actions
tests (windows-latest, beta) tests (windows-latest, beta)
Details
@github-actions
tests (windows-latest, nightly) tests (windows-latest, nightly)
Details
@github-actions
lint
Details
@github-actions
snapshot tests
Details
@codecov
codecov/patch 94.11% of diff hit (target 93.45%)
Details
@codecov
codecov/project 93.46% (+0.01%) compared to 044931d
Details
@smashwilson smashwilson deleted the update-electron-apis branch Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants