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

WORK IN PROGRESS: Add a Page interface #5583

Closed
wants to merge 2 commits into from

Conversation

bep
Copy link
Member

@bep bep commented Jan 2, 2019

This commit adds a new resources/page package with a new Page interface and then use that interface for the core Pages definition (for .Site.Pages etc.).

The above description looks small and isolated. The number of refactorings needed to get there should show that this was very much needed. The old Page struct was getting too big; it was hard to understand and maintain, but most importantly hard/impossible to extend to other non-file sources (see #5074).

The new Page interface is still too big (see below), but that it now is an interface means that it's much more easily composable, which adds flexibility but is also much easier to understand.

A shorter version:

type Page interface {
	resource.Resource
	resource.ContentProvider
	resource.LanguageProvider
	TranslationsProvider
}

With the above, it's much easier to split and reuse the content handling (shortcodes, word counting etc.), language handling (some sources may not have a concept of language/translations, so we provide a default implementation) etc.

And having all the Page types in its own package (Page, Pages, WeightedPageetc.) means it is much easier to get proper test coverage without having to create a new Hugo site for every test case.

One of the primary goals of this commit is to not break anything. One relevant comment would be that the embedded source.File interface is now moved to a File() method. A Page isn't a file, and all the file related methods, mostly used for the build logic, very much cluttered the API. Luckily the docs on Gohugo.io is mostly on the form .File.Dir. The .Dir variants are kept for now and deprecated.

We really need this for #5074

@bep bep force-pushed the pages-from-data-2 branch 11 times, most recently from 47056a7 to 6aa727e Compare January 4, 2019 18:30
@bep
Copy link
Member Author

bep commented Jan 5, 2019

@spf13 @regisphilibert @digitalcraftsman @moorereason @anthonyfok @budparr and gang. This is a rather technical PR (see the description above), and it's very much a work in progress (I'm not too far away), but if you have some fundamental objections to this, now is the time to speak out (also if you see something obviously wrong in the code, but remember: This is WIP, so I probably know about it).

I kind of wish I had done this earlier, but it seemed like such a daunting task (it was/is), but I have no realistic hope of doing #5074 without first fixing this.

@regisphilibert
Copy link
Member

Its hard for me to understand what this would change (if any) from the user standpoint.

@moorereason
Copy link
Contributor

What you've done so far makes sense. My only concern is from a larger design standpoint. I was thinking we would be targeting the i/o (afero/vfs) layer instead of completely abstracting away the Page itself. But like I said, I haven't spent time researching this, so I'm going to take your word for it. 👍

Also, I agree with you about splitting Page out into its own package for testing purposing (even if it's not an interface). It's a huge chore, but hugolib could use more of this. 😁

@bep
Copy link
Member Author

bep commented Jan 5, 2019

I was thinking we would be targeting the i/o (afero/vfs) layer instead of completely abstracting away the Page itself.

Sure, I have thought similar myself -- but in the end, breaking up a machine to get the bolts needed so you can build the machine ... The problem here, however, is not where you choose to integrate any new page source, but that we have a hugolib package with a Page struct that is so tightly coupled with everything, that even thinking about adding more on top of it (I dread the thought of adding "if source == "wordpress" or something in there ...), is making my head spin. So, I'm convinced that the route I'm taking is at least on a conceptual level, the right one.

@bep bep force-pushed the pages-from-data-2 branch 14 times, most recently from 4180a1f to 9a113da Compare February 17, 2019 10:17
@bep bep force-pushed the pages-from-data-2 branch 10 times, most recently from 9080d84 to b89855a Compare February 19, 2019 20:33
@bep bep force-pushed the pages-from-data-2 branch from bbdcd15 to f7cde1d Compare February 19, 2019 21:38
@bep bep closed this Feb 19, 2019
@bep bep deleted the pages-from-data-2 branch February 19, 2019 22:14
@github-actions
Copy link

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 Jan 30, 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.

3 participants