-
Notifications
You must be signed in to change notification settings - Fork 70
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
Convert to use the electron-installer-common ElectronInstaller class #174
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great except for minor things. The changes are more towards the tests. For example, the test 'with an app with a scoped name' is not needed anymore since that was moved to -common
.
I have mixed feelings about 'move LICENSE to Debian-specific location' since that was also moved to -common
.
If you're planning on fixing the tests in a different PR, let me know and I'll change this to "approve". Also, should we add codecov here too?
I'll delete the tests here.
In a different PR, I'd say. |
I fixed the too-long-line & removed some duplicate tests. |
An implementation using electron-userland/electron-installer-common#13. It ends up being more lines of code because there are some other refactors in there, namely adding
getMaintainer()
,normalizeDescription()
, andnormalizeProductDescription()
.TODO
electron-installer-common
version in this PR