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

MYST-163 Add `golint` package #84

Merged
merged 1 commit into from Jan 5, 2018

Conversation

Projects
None yet
3 participants
@donce
Copy link
Contributor

commented Jan 4, 2018

./bin/lint prints linter warnings.

Solves #42.

@donce donce changed the title Add `golint` package MYST-163 Add `golint` package Jan 4, 2018

glide.yaml Outdated
@@ -14,6 +14,9 @@ import:
version: ^1.1.0
- package: github.com/mitchellh/go-homedir
version: b8bc1bf767474819792c23f32d8286a45736f1c6
- package: github.com/golang/lint

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 4, 2018

Member

Should not it be under testImport - just for DEV environment

This comment has been minimized.

Copy link
@donce

donce Jan 5, 2018

Author Contributor

Yes, fixed.

bin/lint Outdated
@@ -0,0 +1 @@
go list ./... | xargs golint

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 4, 2018

Member
  1. Would it be more efficient to call lint only once, instead per-package
  2. Seems like we check style on vendor also, see how bin/test is made

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 4, 2018

Member
  1. Would be nice to actually use installed dependency in helper bin/lint
    e.g go run vendor/github.com/golang/lint/golint/*.go nat

What google says about best practices about that?

This comment has been minimized.

Copy link
@donce

donce Jan 5, 2018

Author Contributor
  1. Would it be more efficient to call lint only once, instead per-package

It is called only once - golint receives all packages as parameters list at once, using xargs

  1. Seems like we check style on vendor also, see how bin/test is made

No we don't, here's an output of go list ./...:

github.com/mysterium/node
github.com/mysterium/node/bytescount_client
github.com/mysterium/node/client_connection
github.com/mysterium/node/client_promise
github.com/mysterium/node/client_promise/dto
github.com/mysterium/node/cmd/mysterium_client
github.com/mysterium/node/cmd/mysterium_client/command_run
github.com/mysterium/node/cmd/mysterium_fake
github.com/mysterium/node/cmd/mysterium_monitor
github.com/mysterium/node/cmd/mysterium_monitor/command_run
github.com/mysterium/node/cmd/mysterium_monitor/command_run/node_provider
github.com/mysterium/node/cmd/mysterium_server
github.com/mysterium/node/cmd/mysterium_server/command_run
github.com/mysterium/node/communication
github.com/mysterium/node/communication/nats
github.com/mysterium/node/communication/nats_dialog
github.com/mysterium/node/communication/nats_discovery
github.com/mysterium/node/datasize
github.com/mysterium/node/identity
github.com/mysterium/node/ipify
github.com/mysterium/node/money
github.com/mysterium/node/nat
github.com/mysterium/node/openvpn
github.com/mysterium/node/openvpn/service_discovery
github.com/mysterium/node/openvpn/service_discovery/dto
github.com/mysterium/node/openvpn/session
github.com/mysterium/node/server
github.com/mysterium/node/server/dto
github.com/mysterium/node/service_discovery/dto
github.com/mysterium/node/session
github.com/mysterium/node/state_client
github.com/mysterium/node/tequilapi
github.com/mysterium/node/tequilapi/endpoints
github.com/mysterium/node/tequilapi/utils
github.com/mysterium/node/tequilapi/validation
github.com/mysterium/node/utils/file

This comment has been minimized.

Copy link
@donce

donce Jan 5, 2018

Author Contributor
  1. Would be nice to actually use installed dependency in helper bin/lint
    e.g go run vendor/github.com/golang/lint/golint/*.go nat

Good point, updated.

@donce donce force-pushed the feature/MYST-163-golint branch from 90d4f7e to b0ee083 Jan 5, 2018

@donce

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2018

PR is ready to be reviewed 💰

@shroomist

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

perhaps we shouldn't remove unused packages and bump versions in yaml.lock with this ticket.

@Waldz

Waldz approved these changes Jan 5, 2018

@Waldz Waldz merged commit 1c0d0c4 into master Jan 5, 2018

@Waldz Waldz deleted the feature/MYST-163-golint branch Jan 5, 2018

@Waldz

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

+100 We have a Linter!
Lets start clearing errors ;)

@Waldz Waldz referenced this pull request Jan 5, 2018

Closed

Update glide.lock #85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.