Skip to content

Conversation

@mwbrooks
Copy link
Member

Summary

This pull request enables the linter staticcheck rule QF1008: 'Omit embedded fields from selector expression'.

This rule ensures that we omit embedded fields from an expression. For example, the type TriggerUpdateRequest has the embedded field TriggerRequest:

type TriggerUpdateRequest struct {
	TriggerRequest
	TriggerID string `json:"trigger_id"`
}

And the following two expressions are equal in Golang:

  • triggerUpdateRequest.TriggerRequest.Inputs = something
  • triggerUpdateRequest.Inputs = something

Hedging a guess, we only need to use the embedded field when there is a naming conflict. 🤔

Requirements

@mwbrooks mwbrooks added this to the Next Release milestone May 15, 2025
@mwbrooks mwbrooks self-assigned this May 15, 2025
@mwbrooks mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels May 15, 2025
@mwbrooks mwbrooks marked this pull request as ready for review May 15, 2025 02:47
@mwbrooks mwbrooks requested a review from a team as a code owner May 15, 2025 02:47
@codecov
Copy link

codecov bot commented May 15, 2025

Codecov Report

Attention: Patch coverage is 56.52174% with 10 lines in your changes missing coverage. Please review.

Project coverage is 63.31%. Comparing base (09d4247) to head (b8ea6ee).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/api/datastore.go 27.27% 7 Missing and 1 partial ⚠️
internal/api/workflows.go 66.66% 1 Missing ⚠️
internal/pkg/platform/deploy.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
- Coverage   63.32%   63.31%   -0.02%     
==========================================
  Files         211      211              
  Lines       22282    22282              
==========================================
- Hits        14110    14107       -3     
- Misses       7085     7086       +1     
- Partials     1087     1089       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@mwbrooks Thank so much for sharing the reasoning of this change!

For me this makes it more clear that certain structs have fields attached and perhaps this makes for better objects overall? Let's revisit this after some time developing 👾

result, err := c.ExportAppManifest(ctx, "token", "A0123456789")
require.NoError(t, err)
require.Equal(t, "example", result.Manifest.AppManifest.DisplayInformation.Name)
require.Equal(t, "example", result.Manifest.DisplayInformation.Name)
Copy link
Member

Choose a reason for hiding this comment

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

This reads sososo much better to me 📚🥹✨

@mwbrooks mwbrooks merged commit 29a9da0 into main May 16, 2025
6 checks passed
@mwbrooks mwbrooks deleted the mwbrooks-lint-qf1008 branch May 16, 2025 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants