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

Updated the README to show a working example for the latest version (Closes #52) #53

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

yakkomajuri
Copy link
Contributor

The README hadn't been updated for over a year.

Hence, the example provided does not actually work with the most recent version of the library.

I updated the README to show a full working example compatible with the most recent version of the library, which requires Context to be passed to the methods of Client.

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #53 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #53   +/-   ##
=======================================
  Coverage   38.25%   38.25%           
=======================================
  Files           7        7           
  Lines         677      677           
=======================================
  Hits          259      259           
  Misses        395      395           
  Partials       23       23           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c516e4f...8650689. Read the comment docs.

@adamhicks
Copy link
Collaborator

LGTM 👍

import luno "github.com/luno/luno-go"
import (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest combining the imports:

import (
 "log"
 "context"
 "time"

 "github.com/luno/luno-go"
)

README.md Outdated
"log"
"context"
"time"
)

lunoClient := luno.NewClient()
Copy link
Collaborator

Choose a reason for hiding this comment

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

While you're at it, would you mind putting this in a func main() { } ?

README.md Outdated
if err != nil {
log.Fatal(err)
}
log.Println(res)
```

Remember to substitute `<API_KEY_ID>` and `<API_KEY_SECRET>` for your own Id and Secret.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nipick: lowercase id + secret

README.md Outdated
And then access them in Go like so:

```go

Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: remove empty line

README.md Outdated

var API_KEY_ID string = os.Getenv("LUNO_API_ID")
var API_KEY_SECRET string = os.Getenv("LUNO_API_SECRET")

Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: remove empty line

var API_KEY_SECRET string = os.Getenv("LUNO_API_SECRET")

```

Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: remove two empty lines

README.md Outdated
if err != nil {
log.Fatal(err)
}
log.Println(res)
```

Remember to substitute `<API_KEY_ID>` and `<API_KEY_SECRET>` for your own Id and Secret.

It is also recommended to set these as environment variables rather than include them in plaintext. You can do this on `BASH` with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest: We recommend using environment variables rather than including your credentials in plaintext. In bash you do so as follows:

README.md Outdated
It is also recommended to set these as environment variables rather than include them in plaintext. You can do this on `BASH` with:

```
$ set LUNO_API_ID="<API_KEY_ID>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

set -> export

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this one was a brain fart on my end...

@yakkomajuri
Copy link
Contributor Author

@neilgarb Should be good now

Copy link
Collaborator

@neilgarb neilgarb left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks!

@neilgarb neilgarb merged commit a292173 into luno:master Jul 13, 2020
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.

None yet

3 participants