Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Minor cleanups #1306

Merged
merged 19 commits into from
Feb 5, 2019
Merged

Minor cleanups #1306

merged 19 commits into from
Feb 5, 2019

Conversation

gubatron
Copy link
Contributor

@gubatron gubatron commented Nov 21, 2018

Here's a few low hanging fruit I could find, broken down into individual commits in case you don't want to merge all and you want to cherry pick some.

Fixed:

  • Redundant parenthesis in expressions
  • Redundant type declarations
  • Redundant aliases importing packages
  • package/variable name collisions

Cheers.

Some Go/Jetbrains GoLand help needed

PS: Is anybody using Jetbrains Goland IDE?

if so, are you having issues with the vendor folder excluding all the gx/ipfs/<IP_HASH>/package imports? I noticed that the vendor folder has all those packages there, but the IDE is having issues finding them even though I have them. Played with GOPATH in the project to no success.

screen shot 2018-11-21 at 1 40 23 pm

the folders are there though, not sure why it'd be able to find all the other vendor packages and not the gx/ipfs ones

screen shot 2018-11-21 at 1 42 29 pm

Once I get that to work I'll be able to find lots of things to cleanup, unused variables, parameters, and more.

EDIT: Found cause to my dependencies issue. I had manually git cloned the project instead of working on the folder automatically cloned by go inside GOPATH/src/OpenBazaar/openbazaar-go.

I've submitted another PR with some documentation notes for linux/mac users new to golang like myself.

@gubatron gubatron changed the title Minor cleanups [WIP] Minor cleanups Nov 22, 2018
@drwasho
Copy link
Member

drwasho commented Nov 23, 2018

Awesome, thanks @gubatron !

@coveralls
Copy link

coveralls commented Nov 27, 2018

Coverage Status

Coverage remained the same at 34.074% when pulling 50d6d63 on gubatron:minor-cleanups into 0ffe952 on OpenBazaar:master.

@gubatron gubatron changed the title [WIP] Minor cleanups Minor cleanups Nov 27, 2018
@gubatron gubatron changed the title Minor cleanups [WIP] Minor cleanups Nov 27, 2018
@gubatron
Copy link
Contributor Author

I think I need some help getting the travis-ci build to pass.

The travis build fails saying that 3 files are not goimports-ed.
I installed golangci-lint and goimports to try and replicate on my end.

I've tried goimports on the files, but I don't get any diffs on the files:

  • cmd/gencerts.go
  • repo/migrations/Migration007_test.go
  • repo/migrations/Migration0078test.go

screen shot 2018-11-29 at 6 12 45 am

When I run golangci-lint locally, I only get lint errors on long lines, but nothing like travis:
screen shot 2018-11-29 at 6 26 47 am

@gubatron gubatron changed the title [WIP] Minor cleanups Minor cleanups Dec 5, 2018
@gubatron
Copy link
Contributor Author

gubatron commented Dec 5, 2018

This should be ready for revision.
Cheers.

@placer14
Copy link
Member

Thanks for being patient @gubatron. I'll do my best to get this reviewed in the next day or so. Really appreciate the contribution.

@gubatron
Copy link
Contributor Author

Thanks for letting me know it hasn't been forgotten! I understand how it is.

@cpacia
Copy link
Member

cpacia commented Jan 8, 2019

This looks good to merge after release

Did not use 'peerID' as it's already being used as a parameter to the function
pb not redundant as package is actually called namesys.pb
pb not redundant as package is actually called namesys.pb

compilation works once again
Should save memory in the event the slice isn't used right away
@gubatron
Copy link
Contributor Author

rebased once again, cheers.

Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Really appreciate your patience and effort on this PR, @gubatron. LGTM

@placer14 placer14 merged commit 6ffe3c7 into OpenBazaar:master Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants