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

Enforce linter on all files and packages #641

Merged
merged 7 commits into from
Jul 30, 2018
Merged

Conversation

remi-stripe
Copy link
Contributor

This enforces the linter on all files and packages and fixes all the current errors

  • Fix the makefile to run golint ./...
  • Add comments to all const blocks to describe what they represent
  • Fix comments on all structs and exported methods
  • A few breaking changes due to naming of properties or variables:
    • Account resource where IdentityVerificationDetailsCodeScanIdCountryNotSupported is now IdentityVerificationDetailsCodeScanIDCountryNotSupported
    • Account resource where IdentityVerificationDetailsCodeScanIdTypeNotSupported is now IdentityVerificationDetailsCodeScanIDTypeNotSupported
    • BitcointReceiver resource where BitcoinUri is now BitcoinURI.
    • IssuingAuthorization resource where NetworkId is now NetworkID

r? @brandur-stripe
cc @stripe/api-libraries

@remi-stripe
Copy link
Contributor Author

Flagging that there's one left that I'm not sure how to fix

client/api.go:5:2: should not use dot imports

@remi-stripe
Copy link
Contributor Author

Flagging that I got a "success" email for the build (2 really) though I expected it to fail since we now run the linter on the whole library.

@brandur-stripe
Copy link
Contributor

brandur-stripe commented Jul 30, 2018

Flagging that there's one left that I'm not sure how to fix

It just doesn't like this line:

import (
	. "github.com/stripe/stripe-go"

It's considered poor practice to use . because you're polluting the package's global namespace rather than referring to names via a package symbol. Try changing this to a:

import (
	stripe "github.com/stripe/stripe-go"

And putting stripe. in front of any references that broke.

Flagging that I got a "success" email for the build (2 really) though I expected it to fail since we now run the linter on the whole library.

You still need to specify the -set_exit_status flag for Golint. It defaults to exiting with 0 even if linting problems were found.

@brandur-stripe
Copy link
Contributor

And also: wow :) This is a serious piece of work!!

@remi-stripe
Copy link
Contributor Author

@brandur-stripe ha I tried a few things but did not think to rename the .! Thanks for the quick answer.

PTAL

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Left a few comments, but looks amazing!

I'm pleasantly surprised by how many legitimate documentation problems the linter picked up. It looks like this'll be a huge improvement for preventing documentation-related problems from being introduced in the future.

ptal @remi-stripe

product.go Outdated
// PackageDimensions represents the dimension of a product or a sku from the
// perspective of shipping.
// PackageDimensionsParams represents the set of parameters for the the dimension of a
// product or a sku from the perspective of shipping .
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we capitalize "SKU" here, just for completeness?

sku.go Outdated
const (
SKUInventoryValueInStock SKUInventoryValue = "in_stock"
SKUInventoryValueLimited SKUInventoryValue = "limited"
SKUInventoryValueOutOfStock SKUInventoryValue = "out_of_stock"
)

// InventoryParams is the set of parameters allowed as inventory on a SKU
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we punctuate the end of this sentence?

ErrNotSigned = errors.New("Webhook has no Stripe-Signature header")
ErrInvalidHeader = errors.New("Webhook has invalid Stripe-Signature header")
ErrTooOld = errors.New("Timestamp wasn't within tolerance")
ErrNoValidSignature = errors.New("Webhook had no valid signature")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you lowercase the first letter of all these errors?

(Surprised that Golint didn't catch this — that may be a bug.)

params.go Outdated
@@ -16,6 +16,7 @@ import (
// Public constants
//

// Describes possible values for type of pagination when using a List API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change this one to:

// Contains constants for the names of parameters used for pagination in list APIs.

@remi-stripe
Copy link
Contributor Author

@brandur-stripe Thanks for the quick review! Just fixed all of those, PTAL!

@brandur-stripe
Copy link
Contributor

Thanks Remi! LGTM.

@brandur-stripe brandur-stripe merged commit 4edbd4e into master Jul 30, 2018
@brandur-stripe brandur-stripe deleted the remi-fix-linter branch July 30, 2018 20:47
@brandur-stripe
Copy link
Contributor

Released as 38.0.0.

Well, I hate to do two major versions in one day, but I figure that we may as well release this one while very few people have picked up version 37.

nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
Bumps [selenium-webdriver](https://github.com/SeleniumHQ/selenium) from 4.3.0 to 4.4.0.
- [Release notes](https://github.com/SeleniumHQ/selenium/releases)
- [Changelog](https://github.com/SeleniumHQ/selenium/blob/trunk/rb/CHANGES)
- [Commits](SeleniumHQ/selenium@selenium-4.3.0...selenium-4.4.0)

---
updated-dependencies:
- dependency-name: selenium-webdriver
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants