-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix release display and correct paging #2080
Conversation
Continuation to #2035 |
b0236c7
to
afc8804
Compare
} | ||
|
||
// GetReleaseCountByRepoID returns the count of releases of repository | ||
func GetReleaseCountByRepoID(repoID int64, includeDrafts bool) (int64, error) { | ||
func (opts *FindReleasesOptions) toConds(repoID int64) builder.Cond { |
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 not let repoID
as a field of FindReleasesOptions?
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.
At first I did that but than it looked strange as func name is GetReleasesByRepoID
. And also repoID
is mandatory condition
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 think FindReleases
and CountReleases
maybe a new name?
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.
But repoID is still allways required field and that will not easy be understandable by function signature
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.
seems right.
// GetReleasesByRepoIDAndNames returns a list of releases of repository according repoID and tagNames. | ||
func GetReleasesByRepoIDAndNames(repoID int64, tagNames []string) (rels []*Release, err error) { | ||
// GetReleasesByRepoID returns a list of releases of repository. | ||
func GetReleasesByRepoID(repoID int64, opts FindReleasesOptions, page, pageSize int) (rels []*Release, err error) { |
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 above and page and pageSize is also could be fields of FindReleasesOptions
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.
Because it's not really related to conditions and toConds can't use them anyway
LGTM |
LGTM |
Fixes #1863