-
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
added vote feature on comments (upvote, downvote) #22
base: master
Are you sure you want to change the base?
Conversation
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.
Overall LGTM, can you also take a look at the ci?
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.
I read the change carefully, very nice change! Thank you, Gursharan!
The orm usage was still not perfect, could you please have another change?
} | ||
|
||
type CommentModelVote struct { | ||
CreatedAt time.Time |
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.
As we want to provide a best practice
, can we use http://gorm.io/docs/conventions.html?
like
gorm.Model |
UserID uint `gorm:"primary_key;auto_increment:false"` | ||
CommentID uint `gorm:"primary_key;auto_increment:false"` |
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.
The id might not be uint in some database, as we have used the orm.
We should use orm feature to make it support more database.
http://gorm.io/docs/conventions.html#ID-as-Primary-Key
DownVote: vote.DownVote, | ||
} | ||
err := db.Create(&commentVote).Error | ||
fmt.Printf("\n\nthis is in CreateCommentVote: %+v\n\n", commentVote) |
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.
If a error happend, commentVote will be what?
Remove it or
if err != nil {
...
}
c.JSON(http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
userId := c.MustGet("my_user_id").(uint) |
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.
Can we use users.UserModel here?
We should not assume the user id is uint, so that someone others will only need change 1 line here and all code could be reuse.
ID uint `gorm:"primary_key"` |
We could get the user information by one retrieve of UserModel.
https://github.com/gothinkster/golang-gin-realworld-example-app/blob/master/articles/routers.go#L136
c.JSON(http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
userId := c.MustGet("my_user_id").(uint) |
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.
ditto
Description string `form:"description" json:"description" binding:"max=2048"` | ||
Body string `form:"body" json:"body" binding:"max=2048"` | ||
Tags []string `form:"tagList" json:"tagList"` | ||
} `json:"article"` | ||
articleModel ArticleModel `json:"-"` | ||
} | ||
|
||
type CommentVoteValidator struct { | ||
CommentID uint `uri:"id" binding:"required"` |
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.
Please use orm feature?
http://gorm.io/docs/conventions.html#ID-as-Primary-Key
@@ -15,13 +18,14 @@ var DB *gorm.DB | |||
|
|||
// Opening a database and save the reference to `Database` struct. | |||
func Init() *gorm.DB { | |||
db, err := gorm.Open("sqlite3", "./../gorm.db") | |||
db, err := gorm.Open("postgres", "host=localhost port=5432 user=postgres dbname=realworld password=postgres") |
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? The sqlite3 is easy for newcomers.
Can you sure a newcomer of this project could run this code in his laptop?
@@ -0,0 +1,13 @@ | |||
module golang-gin-realworld-example-app |
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.
Nice!!! Great change!!
Do we need to change the readme?
https://github.com/gothinkster/golang-gin-realworld-example-app/blame/d5d69550e66cdc5d671a8b704ea20e457917e3f8/readme.md#L58
|
||
// gin "gopkg.in/gin-gonic/gin.v1" | ||
"github.com/gin-gonic/gin" | ||
// "github.com/wangzitian0/golang-gin-starter-kit/articles" |
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.
Remove the unused packages:) thanks!
Username string `form:"username" json:"username" binding:"exists,alphanum,min=4,max=255"` | ||
Email string `form:"email" json:"email" binding:"exists,email"` | ||
Password string `form:"password" json:"password" binding:"exists,min=8,max=255"` | ||
Username string `form:"username" json:"username" binding:"required,alphanum,min=4,max=255"` | ||
Email string `form:"email" json:"email" binding:"required,email"` | ||
Password string `form:"password" json:"password" binding:"required,min=4,max=255"` |
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 need to change it? If it was needed, can we use a small change before the vote feature?
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.
removed vendor system and added go module.
added the vote feature
changed the gin package version