-
Notifications
You must be signed in to change notification settings - Fork 148
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
Fall cleanup #493
Fall cleanup #493
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.
Great cleanup! 🤩 Only a couple of comments/suggestions
@@ -30,23 +36,23 @@ jobs: | |||
- uses: actions/checkout@v2 | |||
with: | |||
submodules: recursive | |||
- name: Install Node.js ${{ matrix.arch }} | |||
uses: niik/setup-node@f9547c97f4c519f71dc42e652d6d2c53f9527c1d |
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.
Didn't notice we used a custom setup-node
🤯
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.
Yeah, and from some super untrustworthy source too!
if (process.stdout) { | ||
process.stdout.on('data', chunk => { | ||
if (chunk instanceof Buffer) { | ||
stdout.push(chunk) | ||
} else { | ||
stdout.push(Buffer.from(chunk)) | ||
} | ||
}) | ||
} |
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.
Can't we use optional chaining here? 🤔
if (process.stdout) { | |
process.stdout.on('data', chunk => { | |
if (chunk instanceof Buffer) { | |
stdout.push(chunk) | |
} else { | |
stdout.push(Buffer.from(chunk)) | |
} | |
}) | |
} | |
process.stdout?.on('data', chunk => { | |
if (chunk instanceof Buffer) { | |
stdout.push(chunk) | |
} else { | |
stdout.push(Buffer.from(chunk)) | |
} | |
}) |
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.
Not yet, the version of prettier that we use in dugite didn't support which is why I did it this way (sorry, should have made a comment or at least a review comment about it) it but I'm planning on bumping prettier in a separate PR (this one got noisy enough)
Co-authored-by: Sergio Padrino <sergio.padrino@gmail.com>
@sergiou87 Would you like me to do a prettier PR targeting this or are you happy to approve this as-is? |
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.
Happy to approve it as is! (sorry, I missed your changes after my review two weeks ago 😓 )
Oh no worries, I got distracted away from it anyway. I should have requested a re-review. Thanks for taking a look! |
Time to give some maintenance love to dugite.
got
dependency in favor of built-in https modulechecksum
dependency in favor of built-in crypto modulerimraf
dependency in favor of built-in recursive remove