-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
New proposal for relaxing Paginate argument #5132
Conversation
My only comment would be to include doc updates in this PR too, if there are plans to merge it. |
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 you adjust re. my minor comments, I'll merge. I'm pondering about maybe moving the Group method to a template func, but that requires some restructuring. Docs can wait.
hugolib/pagination.go
Outdated
return nil, nil | ||
} | ||
|
||
switch seq.(type) { |
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 you do v := seq.(type)
or similar, you an just use v
inside the case switches and it makes the code a little easier to read.
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.
Should be done (I'm new to the go language so do not hesitate to [ask again me to] fix my code)
hugolib/pageGroup.go
Outdated
@@ -296,3 +296,10 @@ func (p Pages) GroupByParamDate(key string, format string, order ...string) (Pag | |||
} | |||
return p.groupByDateField(sorter, formatter, order...) | |||
} | |||
|
|||
// Group creates a PageGroup from a key and a Pages object | |||
func (p *Page) Group(key interface{}, pages interface{}) (PageGroup, 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.
Make pages be Pages
.
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.
Done
But I do not really understand when using interface{} or not (when the argument can have various real types?)
hugolib/pagination.go
Outdated
if l == 0 { | ||
break | ||
} | ||
switch list[0].(type) { |
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.
switch v := list[0].(type)
same comment as above -- avoids casting inside the switches.
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.
Not done as the value of the first element in the array is not important. Perhaps the check is not the good one.
There is a loop just after, iterating over all elements, to cast them one by one. The initial switch is here to handle type errors but perhaps it should be done for all elements.
If there is a better way to do it in go, tell me or fix it directly.
hugolib/pagination.go
Outdated
} | ||
switch list[0].(type) { | ||
case PageGroup: | ||
pagesGroup := make([]PageGroup, len(list)) |
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.
make(PagesGroup, len(list))
is better.
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.
Done
We can now call Paginate after using the 'sort' function: {{ $paginator := .Page.Paginate (sort .Pages "Params.rating") }}
The implementation use a method as using a plain function (in tpl) leads to circular imports The function would need the PageGroup object, hence the Page object. Both are in hugolib/* that import tpl that would need to import hugolib/Page User can now do {{ $cool := .Page.Group "cool" (where .Site.RegularPages "Params.cool" true) }} {{ $blue := .Page.Group "blue" (where .Site.RegularPages "Params.blue" true) }}
User can now do something such as: {{ $cool := .Page.Group "cool" (where .Site.RegularPages "Params.cool" true) }} {{ $blue := .Page.Group "blue" (where .Site.RegularPages "Params.blue" true) }} {{ $paginator := .Page.Paginate (slice $cool $blue) }} {{ range $paginator.PageGroups }} {{ markdownify (printf "## KEY: %s" .Key) }} {{ range .Pages }} {{ partial "lists/partials/blogentry.html" (dict "Page" .) }} {{ end }} {{ end }}
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This proposal is discussed in issue #4865 (with numerous comments in closed PR #4866)