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

Stop using deprecated net.Dialer.DualStack #3520

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Conversation

joanlopez
Copy link
Contributor

What?

It removes all the uses of net.Dialer.DualStack, because now it's deprecated, and the desired behavior (enabled) is now by default. It also slightly modifies few comments to make that more clear.

Why?

Not sure why staticcheck is not complaining, but I guess it's always good idea to stop using a deprecated field, especially in this case that I can't see any pain.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

N/A

@joanlopez joanlopez self-assigned this Dec 29, 2023
@joanlopez joanlopez requested a review from a team as a code owner December 29, 2023 16:19
@joanlopez joanlopez requested review from oleiade and codebien and removed request for a team December 29, 2023 16:19
@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7d131d6) 72.48% compared to head (2641660) 72.50%.

❗ Current head 2641660 differs from pull request most recent head b0e8457. Consider uploading reports for the commit b0e8457 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3520      +/-   ##
==========================================
+ Coverage   72.48%   72.50%   +0.01%     
==========================================
  Files         276      276              
  Lines       20842    20840       -2     
==========================================
+ Hits        15108    15110       +2     
+ Misses       4771     4767       -4     
  Partials      963      963              
Flag Coverage Δ
ubuntu 72.45% <ø> (+<0.01%) ⬆️
windows 72.36% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Thanks @joanlopez. So strange staticcheck doesn't complain. How did you catch it?

@joanlopez
Copy link
Contributor Author

Thanks @joanlopez. So strange staticcheck doesn't complain.

Honestly, I just assumed it was a staticcheck's bug because it's a struct field and/or because it's in the stdlib (or the combination of both), but didn't really care.

But now that you asked 😛, I spent some time looking at existing issues, and also debugging the related code for the sake of curiosity, and I figured out that there's an open issue (dominikh/go-tools#607) that mentions some cases, including that one specifically (instantiating structs - assignment of deprecated fields), and a PR that looks promising to fix it (dominikh/go-tools#1382).

So, I've pinged the linter's author as a reminder (noting that it worths taking a look, using the net.Dialer.DualStack example), and offered my help to get conflicts resolved on that PR's branch in order to get it ready to be reviewed (cause it seems a bit abandoned, and the PR author is marked as busy).

How did you catch it?

Apparently the IDE (linter?) is smarter/better at catching these than the staticcheck linter 🙈

@joanlopez joanlopez merged commit 77bb9ae into master Jan 4, 2024
22 checks passed
@joanlopez joanlopez deleted the remove-dualstack branch January 4, 2024 19:23
@olegbespalov olegbespalov added this to the v0.49.0 milestone Jan 12, 2024
@joanlopez
Copy link
Contributor Author

joanlopez commented Jan 24, 2024

Jfyi @codebien, as you asked, here's an update: dominikh/go-tools#1382

\ - hopefully it'll get fixed sooner rather than later! 💃🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants