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

Unify page look ups #4955

Merged
merged 3 commits into from
Jul 17, 2018
Merged

Unify page look ups #4955

merged 3 commits into from
Jul 17, 2018

Conversation

bep
Copy link
Member

@bep bep commented Jul 17, 2018

This PR will finish the work from #4796. @vassudanagunta I have tested your changes, the benchmarks looks very good. I see some crashes on several of my test sites related to this, but that seems to be simple nil-pointers. The original PR is too big to go the traditional code review path (I don't have time), so I will do some adjustments. I will write a better commit message for your big commit once I understand the scope a little bit better.

@bep
Copy link
Member Author

bep commented Jul 17, 2018

I'm in the process of testing and getting the great work @vassudanagunta has done with .Site.GetPage and a new .Page.GetPage, merged.

While testing this with some existing sites, the breakage before I made some changes, was too big. And there were cases I found where the "ambiguous" error was thrown in "non-ambigous" situations.

I have made the main problematic site build fine now and have reverted the related site-tests to their master-version to get a better sense of the situation.

What I want to discuss is this:

To reduce the amount of breakage in the wild to its minimum, I have reworked .Site.GetPage with some rules:

  • We cannot support more than 2 arguments, i.e. .Site.GetPage "page" "posts" "mypage.md" will now throw an error. I think this is the most uncommon syntax and should be OK. It is an easy fix to change the above to .Site.GetPage "/posts/mypage.md" or similar.
  • .Site.GetPage "home", .Site.GetPage "home" "" and .Site.GetPage "home" "/" will give you the home page. This means that if you have page in root with the name home.md you need to do .Site.GetPage "/home.md" or similar.

Does the above sound ok? /cc @vassudanagunta @kaushalmodi @digitalcraftsman @regisphilibert @RickCogley and gang

@regisphilibert
Copy link
Member

regisphilibert commented Jul 17, 2018

Looks great. I’m not sure of what the PR improves but I’m sure it’s worth a bit of noise in the discourse. Plus a clear message in the relnotes should quiet the noise.

@kaushalmodi
Copy link
Contributor

kaushalmodi commented Jul 17, 2018

@bep

Looks like my sites have all the corner cases for Hugo :P

I use:

{{ with $.Site.GetPage "taxonomy" $type $term }}

here.

So this change will break my site. But as my motto goes, breaking syntax for the greater good is OK as long as there's an alternative. In this case, looks like I can have a (printf "%s/%s" $type $term) to replace those 2 args?


Update: OK, just unbroke my to-be-broken theme :)

@bep
Copy link
Member Author

bep commented Jul 17, 2018

like I can have a (printf "%s/%s" $type $term) to replace those 2 args?

Yes, that would work. I would not call this a corner case -- it is common enough but much less common than the "full path" syntax, but it is easy to fix, and we have no real choice if we want to keep one .Site.GetPage-- which we obviously do.

@bep
Copy link
Member Author

bep commented Jul 17, 2018

, looks like I can have a (printf "%s/%s" $type $term) to replace those 2 args?

That said, you can replace all the 3 args with the above ... if you want.

@bep bep force-pushed the ref-getpage branch 10 times, most recently from ba6722f to 9ee274a Compare July 17, 2018 18:48
This commit unifies the core internal page index for all page kinds.

This enables the `ref` and `relref` shortcodes to support all pages kinds, and adds a new page-relative  `.GetPage` method with simplified signature.

See gohugoio#4147
See gohugoio#4727
See gohugoio#4728
See gohugoio#4728
See gohugoio#4726
See gohugoio#4652
@bep bep force-pushed the ref-getpage branch 3 times, most recently from 3c06a78 to e67c826 Compare July 17, 2018 21:26
bep added 2 commits July 17, 2018 23:50
This commit is a follow up to a recent overhaul of the GetPage/ref/relref implemenation.

The most important change in this commit is the update to `.Site.GetPage`:

* To reduce the amount of breakage in the wild to its minimum, I have reworked .Site.GetPage with some rules:

* We cannot support more than 2 arguments, i.e. .Site.GetPage "page" "posts" "mypage.md" will now throw an error. I think this is the most uncommon syntax and should be OK. It is an easy fix to change the above to .Site.GetPage "/posts/mypage.md" or similar.
* .Site.GetPage "home", .Site.GetPage "home" "" and .Site.GetPage "home" "/" will give you the home page. This means that if you have page in root with the name home.md you need to do .Site.GetPage "/home.md" or similar

This commit also fixes some multilingual issues, most notable it is now possible to do cross-language ref/relref lookups by prepending the language code to the path, e.g. `/jp/posts/mypage.md`.

This commit also reverts the site building tests related to this to "Hugo 0.44 state", to get better control of the changes made.

Closes gohugoio#4147
Closes gohugoio#4727
Closes gohugoio#4728
Closes gohugoio#4728
Closes gohugoio#4726
Closes gohugoio#4652
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants