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

Install Refactor #444

Merged
merged 4 commits into from Jul 29, 2020
Merged

Install Refactor #444

merged 4 commits into from Jul 29, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Jul 23, 2020

This refactor flattens the code as much as possible and fixes an install bug. The conversion did lose one cleanup task (deleting the zip on error), will try and restore that later.

@appilon appilon requested review from radeksimko, a team and paultyng and removed request for radeksimko July 23, 2020 17:13
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

My recent JS/TS experience is rusty, as you know, but the code LGTM.

I just noticed that the code may need formatting:

.on('error', reject)
.end();

.on('end', () => resolve(body));

Also weirdly when I attempt to launch it locally I get the following error - unsure what's the cause:

Screenshot 2020-07-28 at 08 49 06

@appilon
Copy link
Contributor Author

appilon commented Jul 28, 2020

@radeksimko the formatting is Alex Pilon flavoured javascript, I like to indent and align chained methods, I find helps keep track of what "layer" you are in once you start putting callbacks everywhere. I will run the vscode formatter but I don't think it will correct that (javascript does not have strict formatting best practices like Go... sadly).

EDIT: AH I see it does look funny, for some reason that's my editor doing that...

As for the error I have never seen that, what are your repro steps? I will see if I can get that error.

@appilon
Copy link
Contributor Author

appilon commented Jul 28, 2020

@radeksimko I am able to reproduce that error message now

@appilon appilon requested a review from radeksimko July 29, 2020 00:09
@appilon
Copy link
Contributor Author

appilon commented Jul 29, 2020

@radeksimko fixed!

@appilon appilon merged commit 68c4c23 into master Jul 29, 2020
@appilon appilon deleted the install-refactor branch July 29, 2020 15:18
@appilon appilon mentioned this pull request Aug 19, 2020
@ghost
Copy link

ghost commented Aug 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants