-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix printing interfaces on node18 #811
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
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) |
Hello all, I appreciate everybody chiming in on this PR. It's been more than three years since this PR was active, and based on my experience, it's pretty difficult to revive and update a PR after such a long time for a variety of reasons: contributors move on, context is lost, merge conflicts happen, etc. So I decided to create a new PR (#926) to resolve this issue and close this one. We appreciate everybody who contributes to this awesome project and we'll do our best to follow up on issues and PRs as quickly as possible moving forward. Best, |
Changes minor logging output issue
Relevant issues
Fixes #810
Contributor checklist
--help
outputmaster
branchMaintainer checklist