-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix printing interfaces on node18 #811
base: master
Are you sure you want to change the base?
Fix printing interfaces on node18 #811
Conversation
better to testing with nodejs 18 |
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.
lgtm, although I'd just add a check for || details.family === 4
on 229 for simplicity
I would say that this adds clarity right -- code is not just read by computers but also by humans, and node 16 will be EOL at some point, and then this could simply be removed again (but only if its clear that it is a node <= 18 thing). I believe I've granted the maintainers access to the branch, so they can make simplifications as they see fit before merging. |
Thank you for the PR! And for the write access! I just added a small comment. I'm going to add Node 18 testing to master, merge that in, let tests run, then merge |
It would actually be much easier for you to merge in latest master than me, could you whenever you get a chance? |
Should be able to do that tonight
…On Tue, May 31, 2022, 6:09 PM Jade Michael Thornton < ***@***.***> wrote:
It would actually be much easier for you to merge in latest master than
me, could you whenever you get a chance?
—
Reply to this email directly, view it on GitHub
<#811 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACECGJE5R2VQ46W2C3L3CGLVM2E2ZANCNFSM5VNJVFOQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
merged and pushed |
@thornjad - its merged, in case you missed it |
@thornjad - master has been merged |
@thornjad it's been over a month,,,;.; |
Maintainer is asking for funding but hasn't touched this repo all summer (~3 months) |
Changes minor logging output issue
Relevant issues
Fixes #810
Contributor checklist
--help
outputmaster
branchMaintainer checklist