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

go vet cleanup #2182

Merged
merged 1 commit into from
Dec 5, 2022
Merged

go vet cleanup #2182

merged 1 commit into from
Dec 5, 2022

Conversation

tmclane
Copy link
Member

@tmclane tmclane commented Dec 5, 2022

Updated to remove only one line of unreachable code.

@tmclane tmclane requested a review from leaanthony December 5, 2022 17:29
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 5, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c951628
Status: ✅  Deploy successful!
Preview URL: https://4f92d15c.wails.pages.dev
Branch Preview URL: https://feature-govet-cleanup.wails.pages.dev

View logs

Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

Thanks for this @tmclane . Did go vet add in the +build lines as I believe they've been deprecated as of 1.18

@tmclane
Copy link
Member Author

tmclane commented Dec 5, 2022

Ah I forgot we weren't supporting older Go versions any longer.
The goimports adds it automatically I think for backwards-compatibility.
I can take that out.

pkg/menu/styledlabel.go:255:2: unreachable code
@tmclane tmclane force-pushed the feature/govet-cleanup branch from 1bf9aee to c951628 Compare December 5, 2022 20:00
@tmclane tmclane requested a review from leaanthony December 5, 2022 20:01
Copy link
Member

@leaanthony leaanthony left a comment

Choose a reason for hiding this comment

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

That reduced it down to 1 line! Do you know why the return was removed?

@tmclane
Copy link
Member Author

tmclane commented Dec 5, 2022

Because of the default case on line 250 in the switch statement the final line in the function will never fire.

@tmclane
Copy link
Member Author

tmclane commented Dec 5, 2022

I am thinking that perhaps leaving the final return and updating the default case to not do a return might be more appropriate.

@leaanthony
Copy link
Member

It's hard to review on mobile 😅

@leaanthony
Copy link
Member

I think it's ok as is 👍

@leaanthony leaanthony merged commit d2339de into master Dec 5, 2022
@leaanthony leaanthony deleted the feature/govet-cleanup branch December 5, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants