Add style guide for Go #373

infraredgirl · 2020-11-09T21:03:20Z

This change adds a new category for Style guides and a style guide for Go under it.

Notes to reviewers:

The category should be called "Style guides", not "Styleguides", but anchor links don't work properly if I do that. Maybe the platform team can help out with this one.
Totally open to using different icons, layout, section/category names etc. Let me know if you have any suggestions. I am not a designer! 😄
Suggestions of what else to add to the guide content-wise are very welcome - since I am fairly new to the company, I am not yet familiar with all the particularities of our repos.

netlify · 2020-11-09T21:04:17Z

Deploy preview for keen-clarke-470db9 ready!

Built with commit 26b897c

https://deploy-preview-373--keen-clarke-470db9.netlify.app

zackproser · 2020-11-09T21:36:05Z

This is awesome! Thanks for adding it - already learned from it.

yorinasub17

This LGTM! I had a few nits about the organization of the guides but other than that looks great!

yorinasub17 · 2020-11-09T21:55:21Z

_includes/search.html

                <div class="filter" id="gcp"><span class="helper"></span><img src="/assets/img/logos-technologies/gcp-logo.png" alt="aws logo"></div>
-                <div class="filter"id="azure"><span class="helper"></span><img src="/assets/img/logos-technologies/azure-logo.png" alt="aws logo"></div>
+                <div class="filter" id="azure"><span class="helper"></span><img src="/assets/img/logos-technologies/azure-logo.png" alt="aws logo"></div>
+                <div class="filter" id="styleguides"><span class="helper"></span><img src="/assets/img/logos-technologies/style-guides-logo.png" alt="style guides logo"></div>


I'm not sure this fits in this filter, given that it has nothing to do with the cloud. I think what we actually want is the guide to be marked as all clouds, similar to https://github.com/gruntwork-io/gruntwork-io.github.io/blob/master/_posts/2019-08-26-how-to-use-gruntwork-infrastructure-as-code-library.adoc

yorinasub17 · 2020-11-09T21:56:01Z

_posts/2020-11-09-golang-style-guide.adoc

+image: /assets/img/guides/go-logo-blue.png
+excerpt: The style guide for the Go programming language that is used by all Gruntwork-owned code repositories that contain Go code.
+tags: ["go", "golang", "code", "style"]
+cloud: ["styleguides"]


As mentioned above, I think we should actually mark this as all clouds to avoid overloading the cloud filter.

yorinasub17 · 2020-11-09T21:59:24Z

_posts/2020-11-09-golang-style-guide.adoc

+=== Testing
+All files containing business logic must have a corresponding unit test file, and we aim to have 100% test coverage.
+
+Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel` in the test function.


I think https://www.notion.so/gruntwork/Terraform-Module-Style-Guide-78084ab078e4472481b499a5767e136e#759fdeca732647c2a8c019dd1d9bf3c9 should be ported here.

zackproser

🎉

zackproser · 2020-11-09T21:37:58Z

_includes/search.html

                <div class="filter" id="gcp"><span class="helper"></span><img src="/assets/img/logos-technologies/gcp-logo.png" alt="aws logo"></div>
-                <div class="filter"id="azure"><span class="helper"></span><img src="/assets/img/logos-technologies/azure-logo.png" alt="aws logo"></div>
+                <div class="filter" id="azure"><span class="helper"></span><img src="/assets/img/logos-technologies/azure-logo.png" alt="aws logo"></div>
+                <div class="filter" id="styleguides"><span class="helper"></span><img src="/assets/img/logos-technologies/style-guides-logo.png" alt="style guides logo"></div>


NIT: My one piece of feedback here is that even though I was looking for your guide I had a hard time finding it - the </> logo just didn't jump out at me for some reason - and it was harder to parse than its sibling icons which all include text.

Perhaps we should consider creating an icon that says "style guides" or is somehow otherwise more clear?

Totally agree wrt icon, however this issue is moot as I'll move the style guide under all clouds, as described here.

zackproser · 2020-11-09T21:38:45Z

_posts/2020-11-09-golang-style-guide.adoc

+
+=== Repo-specific conventions
+==== terratest
+Note the existence of methods in terratest which are suffixed with the letter `E`, e.g.


@yorinasub17 is there anywhere else we're currently using this pattern that we could include here? I feel like I've seen it elsewhere, too.

I don't think so, but we should add as we find them.

robmorgan · 2020-11-10T06:35:31Z

_posts/2020-11-09-golang-style-guide.adoc

+=== Testing
+All files containing business logic must have a corresponding unit test file, and we aim to have 100% test coverage.
+
+Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel` in the test function.


Suggested change

Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel` in the test function.

Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel()` in the test function.

Should we add parenthesis to reinforce its a function call?

I think it's fine, since we're talking about a function and referring to it by its name.

brikis98

Thanks for putting this together Ana!

A few more items to add:

Default to functional programming practices. That is:
1. Immutability. Prefer returning a new value rather than mutating an existing one.
2. Pure functions. Prefer functions that take all their inputs as function parameters, instead of reading those inputs via side effects (e.g., reading from disk or the network or global vars), and who's entire behavior is to return values (note: errors are values too!), rather than performing side effects (e.g., by writing to disk or the network or global vars). Of course, you can't avoid side effects forever if you want your code to do something useful, but try to do as much of your logic with pure functions as you can, try to pass everything around as explicit parameters and return everything as explicit values, and centralize the side effecting code to a few isolated places.
3. Composition. Build your code out of small, named functions, that you compose together.
4. Examples. Might be worth adding a few examples of the above.
Error handling.
1. Don't panic (hat tip, Douglas Adams). Any method that can have an error should return that error as its final value. This should be passed up through each layer, which can decide what to do with it, all the way up to the very entrypoint of the app (or main) if necessary. You should almost NEVER use panic.
2. Use custom error types. Create your own types that implement the error interface so that error messages are clear and have well-defined types you can check against.
3. Include stack traces. In most of our code, we have to wrap errors with errors.WithStackTrace(e) to add the stack trace. Go annoyingly doesn't do this by default, but without it, sorting out an error can be very tricky.
Comments. We may want to standardize on a comment style. E.g., Every public function foo has a // comment of the style foo <explanation>, where explanation says what foo does, and more importantly, why it does, what assumptions it makes, and other aspects not obvious from the function name.

brikis98 · 2020-11-10T08:50:21Z

_posts/2020-11-09-golang-style-guide.adoc

+image: /assets/img/guides/go-logo-blue.png
+excerpt: The style guide for the Go programming language that is used by all Gruntwork-owned code repositories that contain Go code.
+tags: ["go", "golang", "code", "style"]
+cloud: ["styleguides"]


brikis98 · 2020-11-10T08:50:49Z

_posts/2020-11-09-golang-style-guide.adoc

+endif::[]
+
+== Intro
+This is the style guide for the Go programming language. It aims to help us ensure that the code we write is


Suggested change

This is the style guide for the Go programming language. It aims to help us ensure that the code we write is

This is Gruntwork's style guide for the Go programming language. It aims to help us ensure that the code we write is

brikis98 · 2020-11-10T08:53:07Z

_posts/2020-11-09-golang-style-guide.adoc

+We use the excellent https://golang.org/doc/effective_go.html[Effective Go] guide as the starting point for our style
+guide. Unless explicitly mentioned otherwise, we default to following the conventions outlined in it.
+
+Another helpful resource is the https://github.com/golang/go/wiki/CodeReviewComments[CodeReviewComments] section of the
+Go GitHub wiki page.
+
+Below you will find the list of conventions that differ from the above mentioned documents, either because they are
+specific to Gruntwork, or because we consciously chose to go against what Effective Go and CodeReviewComments recommend.


For some reason, these paragraphs are rendering without the proper vertical whitespace between them: https://deploy-preview-373--keen-clarke-470db9.netlify.app/guides/styleguides/golang-style-guide/#starting-point. @eak12913 @oredavids Could you take a look?

I noticed this when doing the upgrade guide too. I didn't have time to figure out why this was happening, but I did manage to see that the CSS styles were only applying if you use the headings expected in the guides, Intro, Core concepts, Deployment walkthrough, etc. Just wanted to share this in case the info helps shorten the debug cycle.

@eak12913 I noticed this issue still remains:

Note the lack of vertical whitespace between paragraphs. Not urgent, but when you get a chance, could you take a look?

@eak12913 -- test mention as requested.

Thanks!

PR to address the issue here: #379

brikis98 · 2020-11-10T08:53:39Z

_posts/2020-11-09-golang-style-guide.adoc

+
+== Starting point
+We use the excellent https://golang.org/doc/effective_go.html[Effective Go] guide as the starting point for our style
+guide. Unless explicitly mentioned otherwise, we default to following the conventions outlined in it.


Suggested change

guide. Unless explicitly mentioned otherwise, we default to following the conventions outlined in it.

guide. **Unless explicitly mentioned otherwise, we default to following the conventions outlined in Effective Go.**

brikis98 · 2020-11-10T08:54:10Z

_posts/2020-11-09-golang-style-guide.adoc

+
+== Additional conventions
+=== General
+Before committing any Go code, run `go fmt` and `goimports`. We run these as part of the CI build, so this will prevent


IIRC, goimports is a superset of go fmt—it formats the code and imports—so I think running just one is enough. We currently only run go fmt in CI, but in the future, may "upgrade" to goimports.

brikis98 · 2020-11-10T08:56:39Z

_posts/2020-11-09-golang-style-guide.adoc

+Prefer using the `errors` standard library package for handling single errors and `hashicorp/go-multierror` for
+handling multiple errors.


NIT: an example would help here.

NOTE: We want to avoid linking in hashicorp libraries as they use a flavor of copyleft license (albeit weak form) which we currently don't allow use of as a policy. Can't do anything about the usage we already have, but should probably avoid new usage until we resolve that issue.

Given that, we probably shouldn't include recommendations in the style guide that require linking in a hashicorp library.

Ah, that's a good point that totally slipped my mind. Will amend.

brikis98 · 2020-11-10T08:57:05Z

_posts/2020-11-09-golang-style-guide.adoc

+handling multiple errors.
+
+=== Pointer usage
+Prefer using value type over pointers, unless necessary.


I think this needs a bit more context. That is, when are pointers necessary (e.g., when you need to be able to mutate a reference or to return nil as a default).

brikis98 · 2020-11-10T08:59:53Z

_posts/2020-11-09-golang-style-guide.adoc

+Prefer using value type over pointers, unless necessary.
+
+=== Testing
+All files containing business logic must have a corresponding unit test file, and we aim to have 100% test coverage.


I don't think this is true. Our goal isn't 100% test coverage; our goal is to have enough testing to give us confidence that our code works and allow us to update it quickly. That means balancing:

The likelihood of bugs: some code is harder to get right than others, and merits correspondingly more tests.

The cost of bugs: some bugs are much more costly than others—e.g., bugs in payment systems or security functionality—so it makes sense to invest more in testing there.

The cost of tests: some types of tests are cheap & easy to write and maintain, whereas others are very costly and brittle.

In practice, with infra code, which has an exceptional number of side effects, 100% test coverage is rarely a practical goal to aim for.

Makes sense, I'll reword.

brikis98 · 2020-11-10T09:01:18Z

_posts/2020-11-09-golang-style-guide.adoc

+Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel` in the test function.
+
+=== Naming things
+Prefer descriptive names over short ones. In particular, avoid one-letter variable names.


NIT: mention that one-letter variable names are OK in a tiny handful of cases where that has become the idiom (e.g., i as the index in for-loops), but everywhere else, a more descriptive name helps with code understanding and maintenance, at very little cost, given auto-complete in most IDEs/editors.

brikis98 · 2020-11-10T09:02:44Z

_posts/2020-11-09-golang-style-guide.adoc

+https://github.com/gruntwork-io/terratest/blob/master/modules/aws/account.go#L23[GetAccountIdE]. The suffix `E`
+signifies that the method returns an error as the last return value.


Suggested change

https://github.com/gruntwork-io/terratest/blob/master/modules/aws/account.go#L23[GetAccountIdE]. The suffix `E`

signifies that the method returns an error as the last return value.

https://github.com/gruntwork-io/terratest/blob/master/modules/aws/account.go#L23[GetAccountIdE]. Methods that have the suffix `E` return an error as the last return value; methods without `E` mark the test as failed (e.g., via calling `t.Fail()`) instead of returning an error.

infraredgirl · 2020-11-10T19:55:34Z

Addressed all the comments and added quite a bit more content and examples, so would appreciate another review.

Still not sure how to make the category section on the left behave correctly when I put two words there. It sould say "Style Guides" and not "Styleguides".

yorinasub17

Updates LGTM! The one thing that I think we should tweak is the section on errors, where we reference a hashicorp library.

brikis98

LGTM! @eak12913 and @oredavids we'd still appreciate your help on the paragraph spacing issue.

infraredgirl · 2020-11-11T12:31:36Z

Thanks folks for the reviews!

I'll hold off on merging until we get some input on the paragraph spacing and the category naming issues.

bwhaley · 2020-11-12T17:04:31Z

This is fantastic! Nice work. The only feedback I have is to consider mentioning the gruntwork-cli repo which contains a bunch of packages for Go CLI programs, including errors, logging, shell helpers, etc.

eak12913 · 2020-11-17T02:41:50Z

Ugh - I hate hate github notifications. I apologize to all for seeing this so late. Could I ask for a personal favor, if folks need my attention - could you mention me in our Slack channel. This PR has many messages and the specific call out to me got lost in my inbox :-(

eak12913 · 2020-11-17T04:32:48Z

LGTM! @eak12913 and @oredavids we'd still appreciate your help on the paragraph spacing issue.

@brikis98 @infraredgirl --- 2 hours later I found the issue. The problem is with the code in search.js.

TLDR; one part of the code is comparing a kebab cased identifier vs a non kebab cased identifier. Those are identical when there are no spaces, but when spaces are introduced - the comparison fails because you end up with style-guide being compared to style guide and then the section is just not being shown.

I'm going to bed now but I'll push the fix tomorrow!

infraredgirl · 2020-11-17T09:32:45Z

Thanks Eugene, and no worries! GitHub notifications are far from perfect (and this was by no means urgent anyway)!

…ories` which expects an array of strings. If you pass `Style Guides` as a value, then it will treat it like: `["Style", "Guides"]` In the UI we take just the first value out of this array and it will end up just displaying "Style". You either have to provide the value like I did or to do: `categories: ["Style Guides"]`

… that had spaces in it.

eak12913 · 2020-11-17T15:07:23Z

@brikis98 @infraredgirl I pushed my changes to fix the issue. Do you folks want to take a look and approve/merge away as necessary?

infraredgirl · 2020-11-17T16:16:46Z

Hey @eak12913 that looks much better, thanks for your help! The only problem I still see the link in the breadcrumbs is not rendering nor working properly:

Any chance you could help with that also? 😄

…multi word categories.

eak12913 · 2020-11-17T22:30:44Z

Hey @eak12913 that looks much better, thanks for your help! The only problem I still see the link in the breadcrumbs is not rendering nor working properly:
Any chance you could help with that also? 😄

I believe I've fixed this. Please take a look!

eak12913 · 2020-11-17T22:34:09Z

The only bit of slight annoyance is that we're only capitalizing the first word...so it's: "Style guides" in the breadcrumb and not "Style Guides". I'm not sure if this is a problem. It's a little more annoying to fix this with Jekyll - but doable so if it's important I can go back in there and figure it out.

Additionally it should also be possible to do this with CSS - but then that may change the capitalization of a title where we intentionally have "The first thing capitalized but nothing else" into --> "The First Thing Capitalized But Nothing Else"

infraredgirl · 2020-11-18T08:51:58Z

Looks perfect, many thanks @eak12913! (I can definitely live with "Style guides" vs "Style Guides".)

@gruntwork-io/team-iac can one of you give the final seal of approval, please?

brikis98

LGTM!

brikis98 · 2020-11-18T09:18:11Z

BTW, we'll probably want to cross-link to this from our internal Notion pages!

And eventually, migrate our Terraform style guide here...

infraredgirl · 2020-11-18T10:10:37Z

BTW, we'll probably want to cross-link to this from our internal Notion pages!

And eventually, migrate our Terraform style guide here...

Definitely - I'll take care of it.

We should also link to this guide from our open source repos that are heavy on Go (terragrunt, terratest, cloud-nuke, any others?) - I'll do that too.

brikis98 · 2020-11-18T10:11:59Z

Definitely - I'll take care of it.

Thanks! It's not urgent, so feel free to tackle it incrementally, when time permits.

We should also link to this guide from our open source repos that are heavy on Go (terragrunt, terratest, cloud-nuke, any others?) - I'll do that too.

Yup, good idea!

Add style guide for Go

503957e

infraredgirl requested review from eak12913, josh-padnick, oredavids and tsmit291 as code owners November 9, 2020 21:03

infraredgirl self-assigned this Nov 9, 2020

infraredgirl requested review from brikis98, bwhaley, ina-stoyanova, robmorgan and yorinasub17 November 9, 2020 21:04

infraredgirl requested review from ebeneliason, rhoboat and zackproser November 9, 2020 21:04

yorinasub17 reviewed Nov 9, 2020

View reviewed changes

zackproser previously approved these changes Nov 9, 2020

View reviewed changes

robmorgan reviewed Nov 10, 2020

View reviewed changes

robmorgan previously approved these changes Nov 10, 2020

View reviewed changes

brikis98 reviewed Nov 10, 2020

View reviewed changes

Address review comments

8ddd58f

infraredgirl dismissed stale reviews from robmorgan and zackproser via 8ddd58f November 10, 2020 16:40

yorinasub17 reviewed Nov 10, 2020

View reviewed changes

infraredgirl added 2 commits November 11, 2020 13:12

Replace mentions of Hashicorp's multierror with ours

c21bb79

Merge branch 'master' into golang-styleguide

24d0c26

brikis98 previously approved these changes Nov 11, 2020

View reviewed changes

eak12913 added 2 commits November 17, 2020 10:02

Fixed a bug in the search which which resulted in hiding any category…

d5654c7

… that had spaces in it.

eak12913 dismissed brikis98’s stale review via d5654c7 November 17, 2020 15:04

Fixed displaying %20 and broken URL in the breadcrumb of guides with …

26b897c

…multi word categories.

brikis98 approved these changes Nov 18, 2020

View reviewed changes

infraredgirl merged commit fbf8b97 into master Nov 18, 2020

infraredgirl deleted the golang-styleguide branch November 18, 2020 10:12

	Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel` in the test function.
	Unless there's a reason not to, tests should run in parallel. This is done by including a call to `t.Parallel()` in the test function.

	This is the style guide for the Go programming language. It aims to help us ensure that the code we write is
	This is Gruntwork's style guide for the Go programming language. It aims to help us ensure that the code we write is

	guide. Unless explicitly mentioned otherwise, we default to following the conventions outlined in it.
	guide. Unless explicitly mentioned otherwise, we default to following the conventions outlined in Effective Go.

		Prefer using the `errors` standard library package for handling single errors and `hashicorp/go-multierror` for
		handling multiple errors.

		https://github.com/gruntwork-io/terratest/blob/master/modules/aws/account.go#L23[GetAccountIdE]. The suffix `E`
		signifies that the method returns an error as the last return value.

Uh oh!

Add style guide for Go #373

Add style guide for Go #373

Uh oh!

Conversation

infraredgirl commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zackproser commented Nov 9, 2020

Uh oh!

yorinasub17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zackproser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brikis98 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infraredgirl commented Nov 10, 2020

Uh oh!

yorinasub17 left a comment

infraredgirl commented Nov 9, 2020 •

edited

Loading

netlify bot commented Nov 9, 2020 •

edited

Loading

brikis98 left a comment •

edited

Loading

eak12913 commented Nov 17, 2020 •

edited

Loading