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

20250301 go template tweaks #4823

Merged
merged 4 commits into from
Mar 5, 2025
Merged

Conversation

erh
Copy link
Member

@erh erh commented Mar 1, 2025

I just went with the reviewer suggestions, no idea if these are right

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Mar 1, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Mar 1, 2025
Copy link
Member

@penguinland penguinland left a comment

Choose a reason for hiding this comment

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

I'm not on the team that owns this, and defer to Ethan/Nicole.

@@ -49,22 +45,15 @@ func (cfg *Config) Validate(path string) ([]string, error) {
}

type {{.ModelType}} struct {
resource.AlwaysRebuild
Copy link
Member

Choose a reason for hiding this comment

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

👍 Great idea here! AlwaysRebuild should be the default used by 98% of components and services.

)

func main() {
err := realMain()
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this file? It seems to construct the resource and immediately shut everything down. Why bother?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's so people know they can do it and boiler plate

deps := resource.Dependencies{}
// can load these from a remote moachine if you need

cfg := {{.ModuleLowercase}}.Config{}
Copy link
Member

Choose a reason for hiding this comment

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

Many resources have config fields that are required to be non-empty, so this feels wrong. but the resource constructed isn't used, so maybe it's okay...

Copy link
Member Author

Choose a reason for hiding this comment

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

it's perfect

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Mar 1, 2025
Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

looks good to me!

@erh erh merged commit 8d7ef31 into viamrobotics:main Mar 5, 2025
16 checks passed
@erh erh deleted the 20250301-go-template-tweaks branch March 5, 2025 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants