Installer class #13
Installer class #13
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
=========================================
+ Coverage 72.45% 100% +27.54%
=========================================
Files 12 13 +1
Lines 167 179 +12
=========================================
+ Hits 121 179 +58
+ Misses 46 0 -46
Continue to review full report at Codecov.
|
|
I will likely be writing some tests as I |
.
|
Tests are done, an example of an installer using this is in a PR in |
There was a problem hiding this comment.
Sorry, it took so long to review this code. I've been busy and there were so many changes that I needed the extra time.
Everything looks great, this is an amazing job! I also checked the -debian PR to see how things are working, nicely done. I won't have a lot of time these days, so probably won't be able to help to migrate the other modules to use this new version.
| }))).catch(error.wrapError('moving package files')) | ||
| }, | ||
| readElectronVersion: readElectronVersion, | ||
| readElectronVersion, | ||
| readMeta: readMetadata, |
There was a problem hiding this comment.
Is there a reason why we change the name of this function? why not export it as readMetadata or rename the require import to readMeta or rename the function itself?
I know this wasn't changed as part of this PR, but it's just to keep naming export consistency
There was a problem hiding this comment.
I wasn't really interested in making a minor version bump just for that function. I'd prefer to rename the exported function. (In a separate PR.)
malept
force-pushed
the
installer-class
branch
from
44b826d to
017f5c3
Compare
|
@malept I noticed you force-pushed to this PR after I reviewed it. Given that I don't have a lot of time lately, I'll trust that nothing, or not much, changed from my initial review. Even the commit messages look like the same as the old ones. In other news, I created a channel on IRC (Freenode server) called #electron-installer for people to ask for help rather than keep creating issues on Github. I usually stay logged there and in other channels. If you join, let me know to give you ops permissions. I'm not sure if it's worth mentioning on the READMEs of all projects. We could also create a team on Slack or use Gitter or any other one you think might be a good idea to use. |
Yeah, all I did was extract the AVA config commit directly onto master, because I'm going to squash this PR.
I'm usually idling on the #electron channel in the atomio slack, which is one of the (unofficial) support options in the Electron documentation: https://electronjs.org/docs/tutorial/support |
Refactors most of the functions in
src/index.jsinto a structured class. Installer modules can be subclassed.I have successfully converted 4 of the known
electron-installer-*modules to use this:-debian-redhat-flatpak(my fork of it)-windows(a fork of the current-commonPR)I designed
-snapa bit differently, so I'm only using extracted functions, but I think this (large) change is ready for some review.Fixes #8.