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

Add ability to sort by frontmatter parameters #3022

Merged
merged 5 commits into from Feb 10, 2017

Conversation

Projects
None yet
2 participants
@fj
Contributor

fj commented Feb 8, 2017

This allows users to sort by the value of a specified frontmatter key, including anything that would otherwise be available in .Params. Comparisons (and thus the resulting sorting) are done on a string basis, so, for example, xyz123 comes before and not after xyz23.

This is useful for general-purpose sorting for which weight is either not sufficient or where it duplicates information that's already present in the frontmatter. See discussions here.

If the committers are happy with this implementation, I'll also add the correspondingly necessary documentation for this feature.

@bep

This is useful, and if you can address my comments and add a line or two in the docs, I will merge.

Show outdated Hide outdated hugolib/pageSort.go
key := "pageSort.ByParam." + paramsKeyStr
paramsKeyComparator := func(p1, p2 *Page) bool {
v1, _ := p1.Param(paramsKeyStr)

This comment has been minimized.

@bep

bep Feb 9, 2017

Member

You understand that Param will look in the site config if param is not present in the page? I thought a little about this, and I think it makes sense.

@bep

bep Feb 9, 2017

Member

You understand that Param will look in the site config if param is not present in the page? I thought a little about this, and I think it makes sense.

This comment has been minimized.

@fj

fj Feb 9, 2017

Contributor

Yes, that was intentional. That way one can provide site-wide defaults. I'll make sure to document that behavior in the follow-up.

@fj

fj Feb 9, 2017

Contributor

Yes, that was intentional. That way one can provide site-wide defaults. I'll make sure to document that behavior in the follow-up.

Show outdated Hide outdated hugolib/pageSort.go
paramsKeyComparator := func(p1, p2 *Page) bool {
v1, _ := p1.Param(paramsKeyStr)
v2, _ := p2.Param(paramsKeyStr)
s1 := fmt.Sprintf("%v", v1)

This comment has been minimized.

@bep

bep Feb 9, 2017

Member

Use cast.ToString to convert to string. Don't think it will make much practical difference in this case, but then we at least have one way of doing it.

@bep

bep Feb 9, 2017

Member

Use cast.ToString to convert to string. Don't think it will make much practical difference in this case, but then we at least have one way of doing it.

This comment has been minimized.

@fj

fj Feb 9, 2017

Contributor

Agreed.

@fj

fj Feb 9, 2017

Contributor

Agreed.

Show outdated Hide outdated hugolib/pageSort_test.go
@@ -113,6 +113,26 @@ func TestPageSortReverse(t *testing.T) {
assert.True(t, probablyEqualPages(p2, p1.Reverse()))
}
func TestPageSortByParam(t *testing.T) {
unsorted := createSortTestPages(10)
firstValue, _ := unsorted[0].Param("arbitrary")

This comment has been minimized.

@bep

bep Feb 9, 2017

Member

You're missing the test case where the param isn't set. I would say that those pages should come at the end.

@bep

bep Feb 9, 2017

Member

You're missing the test case where the param isn't set. I would say that those pages should come at the end.

This comment has been minimized.

@fj

fj Feb 9, 2017

Contributor

That seems reasonable to me.

@fj

fj Feb 9, 2017

Contributor

That seems reasonable to me.

Show outdated Hide outdated hugolib/pageSort.go
@@ -275,3 +276,20 @@ func (p Pages) Reverse() Pages {
return pages
}
func (p Pages) ByParam(paramsKeyStr string) Pages {

This comment has been minimized.

@bep

bep Feb 9, 2017

Member

Make paramsKeyStr an interface{}-- it comes from the templates, and it may be something else.

@bep

bep Feb 9, 2017

Member

Make paramsKeyStr an interface{}-- it comes from the templates, and it may be something else.

This comment has been minimized.

@fj

fj Feb 9, 2017

Contributor

👍, working on this.

@fj

fj Feb 9, 2017

Contributor

👍, working on this.

@fj

This comment has been minimized.

Show comment
Hide comment
@fj

fj Feb 9, 2017

Contributor

This should address the remarks you had earlier, @bep. Let me know how else I can help.

Contributor

fj commented Feb 9, 2017

This should address the remarks you had earlier, @bep. Let me know how else I can help.

@bep

bep approved these changes Feb 10, 2017

Now it looks right. A line in the doc and this is good o go.

@bep bep merged commit 298ebc3 into gohugoio:master Feb 10, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/wercker Wercker build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment