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

Updates #86

Merged
merged 6 commits into from
Apr 7, 2018
Merged

Updates #86

merged 6 commits into from
Apr 7, 2018

Conversation

fcastilloec
Copy link
Collaborator

This PR does the following:

  • Upgrades dependencies to latest versions
  • Promisify the whole code
  • Fix tests and example

@fcastilloec fcastilloec requested a review from malept March 22, 2018 06:35
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Thanks for doing the work. Having done a similar thing in -debian, I know how tedious that can get.

I took a cursory look - it's a large PR so I'm counting on the tests to help out 😄 I only saw a couple of cosmetic things that should change.

There's inconsistent use of () around a single arrow function parameter. Personally, I'd just get rid of the parentheses.

src/installer.js Outdated
callback(new Error('Error reading package metadata: ' + (err.message || err)))
}
return fs.pathExists(withAsar)
.then((assarExists) => {
Copy link
Member

Choose a reason for hiding this comment

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

asarExists

@malept
Copy link
Member

malept commented Mar 23, 2018

Also, I wonder if it'd be worth making some sort of electron-installer-linux-common or something, seeing as the code is similar between the now four Linux installer modules. (Maybe less so for electron-installer-snap... but that's because I only based it on my experiences maintaining these modules rather than using them as a code start point.)

@fcastilloec
Copy link
Collaborator Author

@malept The majority of the code is just replicating the work you already did on -debian and I've actually done the same thing for -windows. So the work was easier after you did it first.

Regarding the parenthesis, the "inconsistency" comes from me always using airbnb eslint rules which requires parenthesis if the function has a curly brackets body, otherwise no parenthesis. That's why it seems like there's inconsistency, but it follow that rule. Since we don't use that rule, I'll go ahead and remove all parentheses.

Regarding the common module, it might not be a bad idea. The -windows also shares a few similarities but not as much as -debian and -redhat do. It might not be worth it, but just like you, I'm only helping maintain them.

@malept malept merged commit 7ebeb9b into master Apr 7, 2018
@malept malept deleted the updates branch April 7, 2018 22:48
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.

2 participants