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

Tech stack: Go: add Interfaces section #69

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Sep 16, 2022

No description provided.

@render
Copy link

render bot commented Sep 16, 2022

@qdm12 qdm12 force-pushed the qdm12/tech-stack/go/interfaces branch from 406f63b to 4b26694 Compare September 16, 2022 19:37
@qdm12 qdm12 force-pushed the qdm12/tech-stack/go/interfaces branch 6 times, most recently from 5138e41 to 56a8c80 Compare November 9, 2022 09:28
@mpetrunic
Copy link
Member

@qdm12 Might wanna add some go devs in company to review?

@qdm12 qdm12 force-pushed the qdm12/tech-stack/go/interfaces branch from 56a8c80 to 4c78eaf Compare November 9, 2022 09:38
@qdm12 qdm12 force-pushed the qdm12/tech-stack/go/interfaces branch from 4c78eaf to 4f8550c Compare November 9, 2022 09:50

This is also useful such that you can use the narrow interfaces (such as `Getter`) in unexported package-local functions.

### Exported interfaces with exported methods only
Copy link
Member

Choose a reason for hiding this comment

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

@qdm12 could you maybe please elaborate this section even more? Maybe add examples to those 3 paths and some explanation.
And the ideal would be to refer the particular article or a section/post from go.dev.

value := myInterface.(*myImplementation).GetValue()
```

Interfaces should have one or two methods. If you need a larger interface, compose it using smaller interfaces. For example:
Copy link

Choose a reason for hiding this comment

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

curious about the reasoning for this? specifically that an interface should only have 1 or 2 methods?

Copy link

@noot noot left a comment

Choose a reason for hiding this comment

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

overall looks good, just one comment/question

Instead do a type assertion on the interface to call the method. For example:

```go
value := myInterface.(*myImplementation).GetValue()
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to assert whether or not myInterface is of the *myImplementation

Suggested change
value := myInterface.(*myImplementation).GetValue()
impl, ok := myInterface.(*myImplementation)
assert.True(t, ok)
value := impl.GetValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, since it's a test you know in advance what implementation it is. And if not, panicing in the test is fine I'd say.

@ChainSafe ChainSafe deleted a comment from CLAassistant Feb 13, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mpetrunic mpetrunic marked this pull request as draft January 29, 2024 13:00
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.

6 participants