-
Notifications
You must be signed in to change notification settings - Fork 498
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
update package name and upgrade gin #10
base: master
Are you sure you want to change the base?
Conversation
"net/http" | ||
"strconv" | ||
|
||
"github.com/gin-gonic/gin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need use the dev version? Version related issue I think govendor
is better and enough, https://github.com/gothinkster/golang-gin-realworld-example-app/blob/master/vendor/vendor.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now gin no longer gopkg style, use govendor fetch github.com/gin-gonic/[email protected]
can get v1.3.x branch not develop version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got, I have half year not write go...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh, thanks a lot!
@@ -6,11 +6,10 @@ import ( | |||
"math/rand" | |||
"time" | |||
|
|||
"github.com/dgrijalva/jwt-go" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure? Might cause error can'f find jwt
? I think we should delete all package in the vendor or GOPATH, and make sure all packages can be installed in a new environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I make sure it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, travis CI should do the work... but the gothinkster's repo didn't get right config... Let's wait Eric reply..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
"net/http" | ||
"strings" | ||
|
||
jwt "github.com/dgrijalva/jwt-go" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Codes LGTM, but please make sure it works fine in new environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, make sure later on.
Thank you very much! It's a very nice PR, I have left some comments. BTW, I don't know weather travis CI works fine, you could have a try on your own project firstly, that will reduce environment and version changing risk. |
@wangzitian0 thanks review, I will retry the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for the nice change!
Eric didn't answer..So, please run the script in .travis.yml
before your merge on your own.
Make sure the tests/other check works..
go test -v ./...
why this is not merged ? |
why not merged? |
@wangzitian0 who got the write access to this repo? please notify him/her to merge this PR, thanks. |
No description provided.