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

chore: Migrate to async/await #97

Merged
merged 1 commit into from
May 4, 2019

Conversation

mindmelting
Copy link
Contributor

BREAKING CHANGE: Support for Node 6 deprecated, now minimum Node 8

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 97.53%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #97   +/-   ##
=======================================
  Coverage   96.84%   96.84%           
=======================================
  Files          17       17           
  Lines         222      222           
=======================================
  Hits          215      215           
  Misses          7        7
Impacted Files Coverage Δ
src/android/generate-manifest-adaptive-icons.js 100% <100%> (ø) ⬆️
src/label/label-image.js 100% <100%> (ø) ⬆️
src/resize/resize-image.js 100% <100%> (ø) ⬆️
src/init/init.js 100% <100%> (ø) ⬆️
src/android/generate-manifest-icons.js 100% <100%> (ø) ⬆️
src/android/find-android-manifests.js 100% <100%> (ø) ⬆️
src/generate.js 100% <100%> (ø) ⬆️
src/ios/generate-iconset-icons.js 100% <100%> (ø) ⬆️
src/ios/find-iconset-folders.js 100% <100%> (ø) ⬆️
src/imagemagick/get-image-width.js 75% <71.42%> (ø) ⬆️

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 fb0856b...0e06998. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #97 into master will increase coverage by 2.22%.
The diff coverage is 98.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   96.84%   99.07%   +2.22%     
==========================================
  Files          17       16       -1     
  Lines         222      216       -6     
==========================================
- Hits          215      214       -1     
+ Misses          7        2       -5
Impacted Files Coverage Δ
src/android/generate-manifest-adaptive-icons.js 100% <100%> (ø) ⬆️
src/label/label-image.js 100% <100%> (ø) ⬆️
src/resize/resize-image.js 100% <100%> (ø) ⬆️
src/utils/file-exists.js 100% <100%> (+14.28%) ⬆️
src/init/init.js 100% <100%> (ø) ⬆️
src/android/find-android-manifests.js 100% <100%> (ø) ⬆️
src/ios/generate-iconset-icons.js 100% <100%> (ø) ⬆️
src/android/generate-manifest-icons.js 100% <100%> (ø) ⬆️
src/generate.js 100% <100%> (ø) ⬆️
src/utils/delete-if-exists.js 100% <100%> (+14.28%) ⬆️
... and 3 more

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 80ce6ba...326d26f. Read the comment docs.

@mindmelting
Copy link
Contributor Author

@dwmkerr Could you take a first review at this? The node10 test failed as one of them went over 10 seconds strangely - if you kick it off again it should work

Copy link
Owner

@dwmkerr dwmkerr left a comment

Choose a reason for hiding this comment

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

Ace! I've left a few comments, some about your changes, some about potential improvements. If you agree with the potential improvements (which go beyond the scope of these changes) then comment and I'll raise separate tickets.

src/init/init.js Show resolved Hide resolved
bin/app-icon.js Outdated Show resolved Hide resolved
src/android/generate-manifest-adaptive-icons.js Outdated Show resolved Hide resolved
bin/app-icon.js Outdated Show resolved Hide resolved
bin/app-icon.js Show resolved Hide resolved
bin/app-icon.js Show resolved Hide resolved
package.json Show resolved Hide resolved
src/generate.js Outdated Show resolved Hide resolved
src/label/label-image.js Show resolved Hide resolved
@mindmelting
Copy link
Contributor Author

@dwmkerr Take another peek - removed the operations code smell, and added node eslint plugin - other refactoring opportunities should probably take place outside of this PR

@dwmkerr
Copy link
Owner

dwmkerr commented May 3, 2019

@mindmelting looking great. Can you squash and re-push? That way you will appear as the author in the commit history. If I squash, it'll create a single commit with me as the author and I'd like to keep you in the history, so that I can auto-generate 'contributor' files in the future. TY for the comments too @tobiasbueschel. The failing test seems intermittent, it might just need a longer timeout for the one which works on adaptive icons...

@mindmelting mindmelting changed the title WIP: chore: Migrate to async/await chore: Migrate to async/await May 4, 2019
Squashed commits:
[2f0b779] chore: Add in node eslint plugin and fix issues
[b6b72ee] chore: Update to async / await
[4bc6b40] chore: Refactor app-icon.js to async/await
[9c000ab] chore: Remove node 6 from build
[6b25ab7] chore: Update appveyor to use node 8
[06ee78c] chore: Updated documentation to reference node 8
[d77cc10] chore: Migrate to async/await

BREAKING CHANGE: Support for Node 6 deprecated, now minimum Node 8
@mindmelting
Copy link
Contributor Author

@dwmkerr This should be squashed correctly now

@dwmkerr dwmkerr merged commit 8266ef4 into dwmkerr:master May 4, 2019
@dwmkerr
Copy link
Owner

dwmkerr commented May 4, 2019

Released as 0.12.0. Even though this a breaking change, I'm not bumping to 1.0.0 yet, I'll stay on a 0.X line as I still expect API changes in the near term. Once things are really settled I'll cut a genuine v1 release.

Thanks for the help @mindmelting - this will make future changes much easier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants