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

fix errors reported by go vet #2954

Merged
merged 4 commits into from
Jul 20, 2023
Merged

fix errors reported by go vet #2954

merged 4 commits into from
Jul 20, 2023

Conversation

onkarvhanumante
Copy link
Contributor

@onkarvhanumante onkarvhanumante commented Jul 20, 2023

  • As of now, the validate.sh script has been executed as part of the PR checks. However, there was an issue with how the go vet command was invoked in the script.

    if $VET; then
    COMMAND="go vet"
    echo "Running: $COMMAND"
    `$COMMAND`
    fi

    image

  • Currently, the go vet command is not checking all the files. This resulted to errors/ warnings not being captured during the PR checks.

  • The solution is to modify the command to go vet ./... which will ensure that all files are thoroughly checked during the PR checks.

    % (go vet ./... 2>&1) | grep -v -E "composite literal uses unkeyed fields"
    # github.com/prebid/prebid-server/amp
    # github.com/prebid/prebid-server/adapters/adoppler
    # github.com/prebid/prebid-server/adapters/huaweiads
    adapters/huaweiads/huaweiads.go:905:14: result of fmt.Errorf call not used
    # github.com/prebid/prebid-server/adapters/rubicon
    adapters/rubicon/rubicon_test.go:293:9: GetRate passes lock by value: github.com/prebid/prebid- 
    server/adapters/rubicon.mockCurrencyConversion contains github.com/stretchr/testify/mock.Mock contains sync.Mutex
    adapters/rubicon/rubicon_test.go:298:9: GetRates passes lock by value: github.com/prebid/prebid- 
    server/adapters/rubicon.mockCurrencyConversion contains github.com/stretchr/testify/mock.Mock contains sync.Mutex
    # github.com/prebid/prebid-server/adapters/taboola
    adapters/taboola/taboola.go:256:13: result of fmt.Errorf call not used
    
  • The plan here is to update the script to execute the go vet ./... command. However, before implementing this change, let's address any previously uncaught errors or warnings. PR makes changes to fix some errors or warnings reported by go vet

oh0387 added 4 commits July 20, 2023 16:11
fixes warning "struct field storedRequest has json tag but is not exported" returned by go vet
fixes warning "GetRate passes lock by value" returned by go vet
fixes warning "channel used with signal.Notify should be buffered" returned by govet
fixes "GetRate passes lock by value: github.com/prebid/prebid-server/adapters.mockConversions contains github.com/stretchr/testify/mock.Mock contains sync.Mutex" warning returned by govet
@gargcreation1992 gargcreation1992 self-assigned this Jul 20, 2023
@Sonali-More-Xandr Sonali-More-Xandr self-assigned this Jul 20, 2023
@@ -52,12 +52,12 @@ type mockConversions struct {
mock.Mock
}

func (m mockConversions) GetRate(from string, to string) (float64, error) {
func (m *mockConversions) GetRate(from string, to string) (float64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this change to make go vet happy, but the existing code is perfectly fine since it's just a mock and doesn't utilize the lock.

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.

4 participants