Skip to content
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

Make Pages.Prev/Next work like the other Prev/Next methods #4500

Closed
bep opened this issue Mar 12, 2018 · 4 comments · Fixed by #6416
Labels
Milestone

Comments

@bep
Copy link
Member

@bep bep commented Mar 12, 2018

See the TODO(bep) comments in 79dd7cb

This would be a potentially breaking change, but these are very much undocumented, but there seems to be 2 discrepancies with these methods:

  • They go round and round, so Pages.Next will never return nil, which make them ... less than useful.
  • They go in the opposite direction of .Next; the semantic is, when the date is used to sort, to navigate to navigate to newer content, .Prev takes you back in time.
@bep bep added this to the v0.38 milestone Mar 12, 2018
@bep bep modified the milestones: v0.38, v0.39 Mar 20, 2018
@bep bep modified the milestones: v0.39, v0.40 Apr 9, 2018
@bep bep modified the milestones: v0.40, v0.41 Apr 20, 2018
@bep bep modified the milestones: v0.41, v0.42 May 4, 2018
@bep bep modified the milestones: v0.42, v0.43 Jun 5, 2018
@bep bep modified the milestones: v0.43, v0.44 Jun 30, 2018
@bep bep modified the milestones: v0.44, v0.45, v0.46 Jul 10, 2018
@bep bep modified the milestones: v0.46, v0.47, v0.48 Aug 3, 2018
@bep bep modified the milestones: v0.48, v0.49 Aug 22, 2018
@bep bep modified the milestones: v0.49, v0.50 Sep 13, 2018
@bep bep modified the milestones: v0.50, v0.51 Oct 6, 2018
@bep bep modified the milestones: v0.51, v0.53 Nov 8, 2018
@bep bep modified the milestones: v0.56, v0.57 Jun 14, 2019
@bep bep modified the milestones: v0.57, v0.58 Jul 31, 2019
@bep bep modified the milestones: v0.58, v0.59 Aug 15, 2019
@bep bep modified the milestones: v0.59, v0.60 Sep 6, 2019
@bep bep modified the milestones: v0.60, v0.59 Oct 11, 2019
@bep bep added the Bug label Oct 11, 2019
@bep

This comment has been minimized.

Copy link
Member Author

@bep bep commented Oct 11, 2019

@kaushalmodi could you take a quick look at my statements in the 2 bullet points above and tell me if my 2 proposed fixes make sense to you.

bep added a commit to bep/hugo that referenced this issue Oct 11, 2019
@kaushalmodi

This comment has been minimized.

Copy link
Member

@kaushalmodi kaushalmodi commented Oct 11, 2019

Ok, so if I understand this correctly, the above 2 bullet points are the issues and and 2 fixes are:

  • Now both return a nil when they reach the end of a slice
  • Now all Next methods move in the same direction, and the Prev methods move in the other direction.

But what's not clear is what that "direction" is.

the semantic is, when the date is used to sort, use .Next to navigate to navigate to newer content, .Prev takes you back in time.

Is this what is now happening consistently for all Next and Prev methods?

@bep

This comment has been minimized.

Copy link
Member Author

@bep bep commented Oct 11, 2019

But what's not clear is what that "direction" is.

The test case below currently fails:

pages :=  site.RegularPages()
p := pages[5]
if p.Next() != pages.Next(p) {
    panic("Next != Next")
}

So, this isn't the first time we had a similar discussion, and I get confused every time I think about it.

The behavior of .Next and .NextInSection is that .Next points to the lesser value (depending on the page sort). This is probably mostly driven by the default descending date (if weight not set) sorting:

  • Default sort, date, then .Next will point to the newer page.
  • If weight is used to sort, then .Next will point to the page with less weight.
  • If sorted by title and titles are A, B, C, then B.Next==A.

E.g. it points in reverse direction if you look at the slice index, but it points upwards (newer ...) when you list them on a page ...

Note that this issue is not about changing the behaviour of .Next ... Hell will freeze over before that happens.

/cc @regisphilibert

bep added a commit to bep/hugo that referenced this issue Oct 11, 2019
bep added a commit to bep/hugo that referenced this issue Oct 11, 2019
bep added a commit to bep/hugo that referenced this issue Oct 11, 2019
bep added a commit to bep/hugo that referenced this issue Oct 11, 2019
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
This is obviously much faster:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See gohugoio#4500
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
This is obviously much faster:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See gohugoio#4500
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
This is obviously much faster for lager data sets:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See gohugoio#4500
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
This is obviously much faster for lager data sets:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See gohugoio#4500
bep added a commit to bep/hugo that referenced this issue Oct 12, 2019
This is obviously much faster for lager data sets:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See gohugoio#4500
@bep bep closed this in #6416 Oct 13, 2019
bep added a commit that referenced this issue Oct 13, 2019
This is obviously much faster for lager data sets:

```bash
name                         old time/op    new time/op    delta
SearchPage/ByWeight-100-4       267ns ± 4%     272ns ± 5%     ~     (p=0.457 n=4+4)
SearchPage/ByWeight-5000-4     10.8µs ± 3%     1.2µs ± 2%  -88.99%  (p=0.029 n=4+4)
SearchPage/ByWeight-10000-4    21.1µs ± 1%     1.4µs ±11%  -93.28%  (p=0.029 n=4+4)
```

See #4500
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.