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

#6 Receive Achievements #34

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

mikekinlock
Copy link

@mikekinlock mikekinlock commented Jul 7, 2022

Changes

#6 Listing Recent Achievements

  • Added logic to receive the 50 recent achievements
  • Added unit test
  • Updated Readme

@mikekinlock mikekinlock changed the title Receive Achievements #6 Receive Achievements Jul 7, 2022
@mikekinlock mikekinlock self-assigned this Jul 7, 2022
```
**Receive Achievements**

You'll need an admin token to make this example work
Copy link
Member

Choose a reason for hiding this comment

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

Use long form of will. Simple language is important in documentation like this, because most people reading the documentation will not be natives and only have limited language proficiency.

}

for _, achievement := range achievements {
// Do something with the achievement
Copy link
Member

Choose a reason for hiding this comment

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

Please add an example like printing the title of the achievement.

for _, achievement := range achievements {
     fmt.Println(achievement.Title)
}

Email string `json:"email"`
}

func (client *Client) GetAchivements() ([]Achievement, error) {
Copy link
Member

Choose a reason for hiding this comment

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

}

func Test_gettingAchievements(t *testing.T) {
server := httpMock("/api/v1/achievements", http.StatusOK, `{
Copy link
Member

Choose a reason for hiding this comment

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

No need for HTTP mocking here. Use a mock of the bonusly Client.

See: https://github.com/groundfoghub/bonusly-sdk-go/blob/main/user_test.go#L170-L181

@@ -0,0 +1,70 @@
package bonusly
Copy link
Member

Choose a reason for hiding this comment

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

Try to use table tests in Go.

See: https://yourbasic.org/golang/table-driven-unit-test/

Email string `json:"email"`
}

func (client *Client) GetAchivements() ([]Achievement, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Create a type GetAchievementsOutput and return a pointer to it.

bodyMessage, readErr := readBody(resp)

if readErr != nil {
return make([]Achievement, 0), readErr
Copy link
Member

Choose a reason for hiding this comment

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

Return nil instead of empty array.

resp, requestErr := client.Do(req)

if requestErr != nil {
return make([]Achievement, 0), requestErr
Copy link
Member

Choose a reason for hiding this comment

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

Return nil instead of empty array.

jsonParsingErr := json.Unmarshal([]byte(bodyMessage), &archivements)

if jsonParsingErr != nil {
return make([]Achievement, 0), jsonParsingErr
Copy link
Member

Choose a reason for hiding this comment

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

Return nil instead of empty array.

Email string `json:"email"`
}

func (client *Client) GetAchivements() ([]Achievement, error) {
Copy link
Member

Choose a reason for hiding this comment

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

use c as receiver name instead of client for consistency.

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.

2 participants