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.

Sign up for GitHub

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

Activate linting for a few packages #593

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Activate linting for a few packages #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
before_install:
# The testify require framework is used for assertions in the test suite
- go get -u github.com/stretchr/testify/require

# Install code coverage / coveralls tooling
# Install lint / code coverage / coveralls tooling
- go get -u golang.org/x/lint/golint
- go get -u golang.org/x/tools/cmd/cover
- go get -u github.com/modocache/gover
- go get -u github.com/mattn/goveralls
Expand Down
13 changes: 12 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
all: test bench vet check-gofmt
all: test bench vet lint check-gofmt

bench:
go test -race -bench . -run "Benchmark" ./form
Expand All @@ -9,6 +9,17 @@ build:
check-gofmt:
scripts/check_gofmt.sh

# We're trying to get linting activated, but there are so many errors that
# we're doing so incrementally instead of pushing it through as part of one
# giant patch.
#
# As new packages get cleaned up, add them here so that we don't regress. When
# all packages are here, switch to a `./...` instead of linting every package
# individually.
lint:
golint -set_exit_status ./account
golint -set_exit_status ./charge

test:
go test -race ./...

Expand Down
31 changes: 19 additions & 12 deletions account/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Package account provides API functions related to accounts.
//
// For more details, see: https://stripe.com/docs/api/go#accounts.
package account

import (
Expand All @@ -7,7 +10,7 @@ import (
"github.com/stripe/stripe-go/form"
)

// Client is used to invoke /account APIs.
// Client is used to invoke APIs related to accounts.
Copy link
Contributor

Choose a reason for hiding this comment

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

is the change before /account is not a valid comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I made this mostly because I feel a little like the particular URLs being accessed by the package are an implementation detail — not necessarily something that needs to be included in the documentation. The docs are still overly terse, but I was intending to basically replace a URL with a resource name.

type Client struct {
B stripe.Backend
Key string
Expand All @@ -18,76 +21,83 @@ func New(params *stripe.AccountParams) (*stripe.Account, error) {
return getC().New(params)
}

// New creates a new account.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand adding comments to a library but I'm not a fan of really short/terse comments like this. I don't know if it's a Go thing, but I wanted to flag just once.
Also, does it do like javadoc where you comment each parameter and returned values and errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I kind of agree, but it's a bit of a compromise — we could go with something much more long-winded, but it has a higher chance of falling out of date more quickly. I went with this instead, and admittedly it's not great documentation, but it will have the advantage of at least forcing documentation for all exported types/functions later on if we're able to successfully activate Golint.

I'm actually up for trying an alternative if you have a good suggestion. One approach is that we could try to copy what's in the reference documentation, but again, I feel like it might be in slight danger of becoming outdated:

To charge a credit card or other payment source, you create a Charge object. If your API key is in test mode, the supplied payment source (e.g., card) won't actually be charged, although everything else will occur as if in live mode. (Stripe assumes that the charge would have completed successfully).

Regarding commenting on each parameter and returned value: no this isn't really a thing in Go. Instead, important parameters and return values are expected to be mentioned in the normal documentation string. You can find some examples of this in the Godoc for the standard library:

https://godoc.org/net/http#Redirect

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually up for trying an alternative if you have a good suggestion. One approach is that we could try to copy what's in the reference documentation, but again, I feel like it might be in slight danger of becoming outdated

I fully agree. That's why I mostly removed all of those when I did the merge. I feel like the methods are self explanatory and devs should look at our official API ref instead though I don't know how Go devs work. For me it's the same as the ongoing PRs to tag things in stripe-php.

But I think your explanation makes sense and I'm happy to move forward with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks that sounds good. I think if we eventually go to auto-generation, all of this could possibly be less big of a deal because we'll be able to easily change the source and distribute updates out to everywhere (after updating all those sources to be a little more language-agnostic of course).

One more possibility: there were direct links before in a couple of these to the associated action in the API reference documentation like:

https://stripe.com/docs/api/go#create_charge

I could put those back in, and at least that might the docstrings a little more helpful. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would punt. More than once we ended up "guessing" the URLs while adding something gated that did not have docs already and it's been not maintained in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

True — that has definitely been a problem in the past. Okay, thanks! I'm going to merge this and we can see how the rest of it goes.

func (c Client) New(params *stripe.AccountParams) (*stripe.Account, error) {
acct := &stripe.Account{}
err := c.B.Call(http.MethodPost, "/accounts", c.Key, params, acct)
return acct, err
}

// Get returns the details of an account.
// Get retrieves the authenticating account.
func Get() (*stripe.Account, error) {
return getC().Get()
}

// Get retrieves the authenticating account.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can anything about the duplicate comments between both calls? Also aren't you missing one New since there should be two of those in the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately no ... Golint expects all exported types/functions to be documented. One thing we could do is to possibly just reference the first invocation from the second so that at least we don't have to duplicate the text (might be less change churn when updates are made in the future):

// New is an alias for Client.New that uses a default client.

Maybe that would better?

Also aren't you missing one New since there should be two of those in the file?

Believe it or not, one of them already had this docstring on it. You can see that the final result looks right if you open the branch's file.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha. In that case I think it's better to document than reference the other one because it would display the right string to the user who needs it.

func (c Client) Get() (*stripe.Account, error) {
account := &stripe.Account{}
err := c.B.Call(http.MethodGet, "/account", c.Key, nil, account)
return account, err
}

// GetByID returns the details of your account.
// GetByID retrieves an account.
func GetByID(id string, params *stripe.AccountParams) (*stripe.Account, error) {
return getC().GetByID(id, params)
}

// GetByID retrieves an account.
func (c Client) GetByID(id string, params *stripe.AccountParams) (*stripe.Account, error) {
path := stripe.FormatURLPath("/accounts/%s", id)
account := &stripe.Account{}
err := c.B.Call(http.MethodGet, path, c.Key, params, account)
return account, err
}

// Update updates the details of an account.
// Update updates an account.
func Update(id string, params *stripe.AccountParams) (*stripe.Account, error) {
return getC().Update(id, params)
}

// Update updates an account.
func (c Client) Update(id string, params *stripe.AccountParams) (*stripe.Account, error) {
path := stripe.FormatURLPath("/accounts/%s", id)
acct := &stripe.Account{}
err := c.B.Call(http.MethodPost, path, c.Key, params, acct)
return acct, err
}

// Del deletes an account
// Del deletes an account.
func Del(id string, params *stripe.AccountParams) (*stripe.Account, error) {
return getC().Del(id, params)
}

// Del deletes an account.
func (c Client) Del(id string, params *stripe.AccountParams) (*stripe.Account, error) {
path := stripe.FormatURLPath("/accounts/%s", id)
acct := &stripe.Account{}
err := c.B.Call(http.MethodDelete, path, c.Key, params, acct)
return acct, err
}

// Reject rejects an account
// Reject rejects an account.
func Reject(id string, params *stripe.AccountRejectParams) (*stripe.Account, error) {
return getC().Reject(id, params)
}

// Reject rejects an account.
func (c Client) Reject(id string, params *stripe.AccountRejectParams) (*stripe.Account, error) {
path := stripe.FormatURLPath("/accounts/%s/reject", id)
acct := &stripe.Account{}
err := c.B.Call(http.MethodPost, path, c.Key, params, acct)
return acct, err
}

// List lists your accounts.
// List returns an iterator that iterates all accounts.
func List(params *stripe.AccountListParams) *Iter {
return getC().List(params)
}

// List returns an iterator that iterates all accounts.
func (c Client) List(listParams *stripe.AccountListParams) *Iter {
return &Iter{stripe.GetIter(listParams, func(p *stripe.Params, b *form.Values) ([]interface{}, stripe.ListMeta, error) {
list := &stripe.AccountList{}
Expand All @@ -102,15 +112,12 @@ func (c Client) List(listParams *stripe.AccountListParams) *Iter {
})}
}

// Iter is an iterator for lists of Accounts.
// The embedded Iter carries methods with it;
// see its documentation for details.
// Iter is an iterator for accounts.
type Iter struct {
*stripe.Iter
}

// Account returns the most recent Account
// visited by a call to Next.
// Account returns the account which the iterator is currently pointing to.
func (i *Iter) Account() *stripe.Account {
return i.Current().(*stripe.Account)
}
Expand Down
33 changes: 16 additions & 17 deletions charge/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// Package charge provides the /charges APIs
// Package charge provides API functions related to charges.
//
// For more details, see: https://stripe.com/docs/api/go#charges.
package charge

import (
Expand All @@ -8,69 +10,69 @@ import (
"github.com/stripe/stripe-go/form"
)

// Client is used to invoke /charges APIs.
// Client is used to invoke APIs related to charges.
type Client struct {
B stripe.Backend
Key string
}

// New POSTs new charges.
// For more details see https://stripe.com/docs/api#create_charge.
// New creates a new charge.
func New(params *stripe.ChargeParams) (*stripe.Charge, error) {
return getC().New(params)
}

// New creates a new charge.
func (c Client) New(params *stripe.ChargeParams) (*stripe.Charge, error) {
charge := &stripe.Charge{}
err := c.B.Call(http.MethodPost, "/charges", c.Key, params, charge)
return charge, err
}

// Get returns the details of a charge.
// For more details see https://stripe.com/docs/api#retrieve_charge.
// Get retrieves a charge.
func Get(id string, params *stripe.ChargeParams) (*stripe.Charge, error) {
return getC().Get(id, params)
}

// Get retrieves a charge.
func (c Client) Get(id string, params *stripe.ChargeParams) (*stripe.Charge, error) {
path := stripe.FormatURLPath("/charges/%s", id)
charge := &stripe.Charge{}
err := c.B.Call(http.MethodGet, path, c.Key, params, charge)
return charge, err
}

// Update updates a charge's properties.
// For more details see https://stripe.com/docs/api#update_charge.
// Update updates a charge.
func Update(id string, params *stripe.ChargeParams) (*stripe.Charge, error) {
return getC().Update(id, params)
}

// Update updates a charge.
func (c Client) Update(id string, params *stripe.ChargeParams) (*stripe.Charge, error) {
path := stripe.FormatURLPath("/charges/%s", id)
charge := &stripe.Charge{}
err := c.B.Call(http.MethodPost, path, c.Key, params, charge)
return charge, err
}

// Capture captures a charge not yet captured.
// For more details see https://stripe.com/docs/api#charge_capture.
// Capture captures a charge that's not yet captured.
func Capture(id string, params *stripe.CaptureParams) (*stripe.Charge, error) {
return getC().Capture(id, params)
}

// Capture captures a charge that's not yet captured.
func (c Client) Capture(id string, params *stripe.CaptureParams) (*stripe.Charge, error) {
path := stripe.FormatURLPath("/charges/%s/capture", id)
charge := &stripe.Charge{}
err := c.B.Call(http.MethodPost, path, c.Key, params, charge)
return charge, err
}

// List returns a list of charges.
// For more details see https://stripe.com/docs/api#list_charges.
// List returns an iterator that iterates all charges.
func List(params *stripe.ChargeListParams) *Iter {
return getC().List(params)
}

// List returns an iterator that iterates all charges.
func (c Client) List(listParams *stripe.ChargeListParams) *Iter {
return &Iter{stripe.GetIter(listParams, func(p *stripe.Params, b *form.Values) ([]interface{}, stripe.ListMeta, error) {
list := &stripe.ChargeList{}
Expand All @@ -85,15 +87,12 @@ func (c Client) List(listParams *stripe.ChargeListParams) *Iter {
})}
}

// Iter is an iterator for lists of Charges.
// The embedded Iter carries methods with it;
// see its documentation for details.
// Iter is an iterator for charges.
type Iter struct {
*stripe.Iter
}

// Charge returns the most recent Charge
// visited by a call to Next.
// Charge returns the charge which the iterator is currently pointing to.
func (i *Iter) Charge() *stripe.Charge {
return i.Current().(*stripe.Charge)
}
Expand Down