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

hugolib: Introduce Page.NextPage and Page.PrevPage #5252

Merged
merged 1 commit into from Sep 26, 2018

Conversation

Projects
None yet
4 participants
@felicianotech
Contributor

felicianotech commented Sep 25, 2018

Introduce new page position variables in order to fix the ordering issue
of .Next and .Prev while also allowing an upgrade path via
deprecation.

.NextInSection becomes .NextPageInSection.
.PrevInSection becomes .PrevPageInSection.

.Next becomes a function returning .PrevPage.
.Prev becomes a function returning .NextPage.

Fixes #1061

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

Yea, this looks like a good compromise. To be clear: Is the Next "newer" in both cases now?

@kaushalmodi what do you think about this?

Member

bep commented Sep 25, 2018

Yea, this looks like a good compromise. To be clear: Is the Next "newer" in both cases now?

@kaushalmodi what do you think about this?

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

Minor comment on the displayed warning.

Right now it shows:

WARNING: Page's .Next is deprecated and will be removed in a future release. Use .PrevPage

To a casual user, that might look like a typo. So may be make it more explicit?:

WARNING: Page's .Next is deprecated and will be removed in a future release. Use .PrevPage (yes, not .NextPage) instead of .Next.


Even though I know of this refactoring, I had to step back and think for a minute if the warning for correct :)

Member

kaushalmodi commented Sep 25, 2018

Minor comment on the displayed warning.

Right now it shows:

WARNING: Page's .Next is deprecated and will be removed in a future release. Use .PrevPage

To a casual user, that might look like a typo. So may be make it more explicit?:

WARNING: Page's .Next is deprecated and will be removed in a future release. Use .PrevPage (yes, not .NextPage) instead of .Next.


Even though I know of this refactoring, I had to step back and think for a minute if the warning for correct :)

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

While try to refactor my theme, I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR? Because I see that I still have to use .Next and .Prev there.

Member

kaushalmodi commented Sep 25, 2018

While try to refactor my theme, I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR? Because I see that I still have to use .Next and .Prev there.

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

Another comment about the warning:

Is it possible to also display the layout file path and line number in the warning. That way, the user know where exactly the refactoring needs to happen.

Member

kaushalmodi commented Sep 25, 2018

Another comment about the warning:

Is it possible to also display the layout file path and line number in the warning. That way, the user know where exactly the refactoring needs to happen.

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

Other than those comments, the PR looks great functionally!

Sample - Now NextPage and NextPageInSection point to the same page when expected.

Summary of suggested changes:

  1. Understand if the .Next/.Prev in .Paginator should be refactored too? Or at least have .NextPage and .PrevPage as aliases to them (no warning).
  2. Make the warning message more explicit.
  3. Add file path and line number to the warning.
Member

kaushalmodi commented Sep 25, 2018

Other than those comments, the PR looks great functionally!

Sample - Now NextPage and NextPageInSection point to the same page when expected.

Summary of suggested changes:

  1. Understand if the .Next/.Prev in .Paginator should be refactored too? Or at least have .NextPage and .PrevPage as aliases to them (no warning).
  2. Make the warning message more explicit.
  3. Add file path and line number to the warning.
@felicianotech

This comment has been minimized.

Show comment
Hide comment
@felicianotech

felicianotech Sep 25, 2018

Contributor

@bep

To be clear: Is the Next "newer" in both cases now?

Yes. In following with the "default sort", if page weight is the same, .NextPage and .NextPageInSection will point to the page that is newer by date. The other two will point to the older page.

@kaushalmodi

To a casual user, that might look like a typo. So may be make it more explicit?

I agree with you completely. Problem is, that message is mostly generated from a helper function however I might be able to squeeze something like that in there. I'll check.

I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR?

I don't know. I honestly did not take Paginator into consideration for this PR. Aside from the naming, do they go in the right direction already? I can take a look at that code and see what's up.

Is it possible to also display the layout file path and line number in the warning. That way, the user know where exactly the refactoring needs to happen.

Not sure, but that's likely a different PR. As I mentioned above, Hugo has a helper function for deprecating things that I'm using. I provide what wa deprecated and what to use instead, and it generates that warning message by itself. So maybe a PR enhancing that function is in order?


Thanks for the feedback. I'll follow up about Paginator soon.

Contributor

felicianotech commented Sep 25, 2018

@bep

To be clear: Is the Next "newer" in both cases now?

Yes. In following with the "default sort", if page weight is the same, .NextPage and .NextPageInSection will point to the page that is newer by date. The other two will point to the older page.

@kaushalmodi

To a casual user, that might look like a typo. So may be make it more explicit?

I agree with you completely. Problem is, that message is mostly generated from a helper function however I might be able to squeeze something like that in there. I'll check.

I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR?

I don't know. I honestly did not take Paginator into consideration for this PR. Aside from the naming, do they go in the right direction already? I can take a look at that code and see what's up.

Is it possible to also display the layout file path and line number in the warning. That way, the user know where exactly the refactoring needs to happen.

Not sure, but that's likely a different PR. As I mentioned above, Hugo has a helper function for deprecating things that I'm using. I provide what wa deprecated and what to use instead, and it generates that warning message by itself. So maybe a PR enhancing that function is in order?


Thanks for the feedback. I'll follow up about Paginator soon.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR?

No. That is certainly not .NextPage -- .NetxPager would be more correct, but that just shows that it must be left alone.

Member

bep commented Sep 25, 2018

I noticed that the .Paginator has .Next and .Prev too. Do they need to be refactored too in this PR?

No. That is certainly not .NextPage -- .NetxPager would be more correct, but that just shows that it must be left alone.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

Also note that we may "switch back" to using "Next/Prev" in a future version. This "stunt" is mainly a way to handle the change of direction.

Member

bep commented Sep 25, 2018

Also note that we may "switch back" to using "Next/Prev" in a future version. This "stunt" is mainly a way to handle the change of direction.

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

Also note that we may "switch back" to using "Next/Prev" in a future version.

+1. I like that plan.

Member

kaushalmodi commented Sep 25, 2018

Also note that we may "switch back" to using "Next/Prev" in a future version.

+1. I like that plan.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

One question: Which one of the two has now "changed direction"?

Member

bep commented Sep 25, 2018

One question: Which one of the two has now "changed direction"?

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

Which one of the two has now "changed direction"?

.Next (now .PrevPage)

Member

kaushalmodi commented Sep 25, 2018

Which one of the two has now "changed direction"?

.Next (now .PrevPage)

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

.Next

OK, then I suggest a slightly different approach to this:

  • We introduce NextPage and PrevPage and deprecate Next/Prev (like we did here)
  • We leave Next/PrevInSection as is, that is: We do not do any depreciation and do not add any NextPageInSection etc.

That will leave us with slightly mismatched names for some versions. But then we can in, say Hugo 0.53 (or something), route Next/Prev to NextPage and just adjust the documentation.

I use PrevInSection on some sites in the wild, and it doesn't seem right that I should have to do anything when it's just a temporary name change.

Member

bep commented Sep 25, 2018

.Next

OK, then I suggest a slightly different approach to this:

  • We introduce NextPage and PrevPage and deprecate Next/Prev (like we did here)
  • We leave Next/PrevInSection as is, that is: We do not do any depreciation and do not add any NextPageInSection etc.

That will leave us with slightly mismatched names for some versions. But then we can in, say Hugo 0.53 (or something), route Next/Prev to NextPage and just adjust the documentation.

I use PrevInSection on some sites in the wild, and it doesn't seem right that I should have to do anything when it's just a temporary name change.

@felicianotech

This comment has been minimized.

Show comment
Hide comment
@felicianotech

felicianotech Sep 25, 2018

Contributor

I pushed a commit to update the deprecation message (thanks @kaushalmodi). I'll rebase before this PR gets merged.

So leave Paginator alone?

We leave Next/PrevInSection as is

Okay, I kind of liked have "page" in the variable name so that the purpose is more explicit than just saying "next". The upside to making this temporary though is that not including the word "page" makes for shorter variables I guess.

I guess I'll get another commit ready to revert the changes to the InSection variables.

Contributor

felicianotech commented Sep 25, 2018

I pushed a commit to update the deprecation message (thanks @kaushalmodi). I'll rebase before this PR gets merged.

So leave Paginator alone?

We leave Next/PrevInSection as is

Okay, I kind of liked have "page" in the variable name so that the purpose is more explicit than just saying "next". The upside to making this temporary though is that not including the word "page" makes for shorter variables I guess.

I guess I'll get another commit ready to revert the changes to the InSection variables.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

The upside to making this temporary though is that not including the word "page" makes for shorter variables I guess.es,

There are several reasons to leave it as is.

  1. It is a method so it has a receiver. Page.Next, Paginator.Next ... i.e. give me the next of "this", so yes, leave the paginator as is.
  2. And it's shorter.
Member

bep commented Sep 25, 2018

The upside to making this temporary though is that not including the word "page" makes for shorter variables I guess.es,

There are several reasons to leave it as is.

  1. It is a method so it has a receiver. Page.Next, Paginator.Next ... i.e. give me the next of "this", so yes, leave the paginator as is.
  2. And it's shorter.
@felicianotech

This comment has been minimized.

Show comment
Hide comment
@felicianotech

felicianotech Sep 25, 2018

Contributor

Okay I pushed a new commit that undoes the deprecation of .NextInSection and .PrevInSection. I left the doc description changes though as it's more informative to users.

If we like this, I can rebase my commits but also @bep, GitHub can do merge rebase commits built-in. Whichever you prefer.

Contributor

felicianotech commented Sep 25, 2018

Okay I pushed a new commit that undoes the deprecation of .NextInSection and .PrevInSection. I left the doc description changes though as it's more informative to users.

If we like this, I can rebase my commits but also @bep, GitHub can do merge rebase commits built-in. Whichever you prefer.

@kaushalmodi

This comment has been minimized.

Show comment
Hide comment
@kaushalmodi

kaushalmodi Sep 25, 2018

Member

@felicianotech This PR looks good for merging from my testing. Thanks for working on this.

Member

kaushalmodi commented Sep 25, 2018

@felicianotech This PR looks good for merging from my testing. Thanks for working on this.

@rdwatters

This comment has been minimized.

Show comment
Hide comment
@rdwatters

rdwatters Sep 25, 2018

Contributor

I use PrevInSection on some sites in the wild, and it doesn't seem right that I should have to do anything when it's just a temporary name change.

If you keep the two new names consistent per @felicianotech ‘s recommendation, you wouldn’t have to do anything with these in the wild, right?

One of the biggest learning curves for me (and from what I’ve noticed on the forums and anecdotally from friends I’ve turned onto Hugo) is that Hugo conventions often aren’t conventional. Hence my request yesterday for consistency around string manipulation, which I understand went over like a lead balloon :(

Contributor

rdwatters commented Sep 25, 2018

I use PrevInSection on some sites in the wild, and it doesn't seem right that I should have to do anything when it's just a temporary name change.

If you keep the two new names consistent per @felicianotech ‘s recommendation, you wouldn’t have to do anything with these in the wild, right?

One of the biggest learning curves for me (and from what I’ve noticed on the forums and anecdotally from friends I’ve turned onto Hugo) is that Hugo conventions often aren’t conventional. Hence my request yesterday for consistency around string manipulation, which I understand went over like a lead balloon :(

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 25, 2018

Member

If you keep the two new names consistent per @felicianotech ‘s recommendation, you wouldn’t have to do anything with these in the wild, right?

Not in my head. I would have to update my sites to use NextPageInSection and PrevPageInSection. Which is making me do work for nothing. And it would also move away from a consistent set of other Next and Prev methods.

Member

bep commented Sep 25, 2018

If you keep the two new names consistent per @felicianotech ‘s recommendation, you wouldn’t have to do anything with these in the wild, right?

Not in my head. I would have to update my sites to use NextPageInSection and PrevPageInSection. Which is making me do work for nothing. And it would also move away from a consistent set of other Next and Prev methods.

hugolib: Introduce Page.NextPage and Page.PrevPage
Introduce new page position variables in order to fix the ordering issue
of `.Next` and `.Prev` while also allowing an upgrade path via
deprecation.

`.NextInSection` becomes `.NextPageInSection`.
`.PrevInSection` becomes `.PrevPageInSection`.

`.Next` becomes a function returning `.PrevPage`.
`.Prev` becomes a function returning `.NextPage`.

Fixes #1061
@felicianotech

This comment has been minimized.

Show comment
Hide comment
@felicianotech

felicianotech Sep 25, 2018

Contributor

Okay I've rebased on master. Once CI tests pass, this PR is ready to go on my end.

Contributor

felicianotech commented Sep 25, 2018

Okay I've rebased on master. Once CI tests pass, this PR is ready to go on my end.

@bep bep merged commit ad705aa into gohugoio:master Sep 26, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

kaushalmodi added a commit to kaushalmodi/hugo-debugprint that referenced this pull request Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment