Support an "expiration" date #726 #2137

Closed
wants to merge 15 commits into
from

Projects

None yet

4 participants

@g3wanghc
Contributor

First time using goLang and first time contribution. Please be gentle. :V

@CLAassistant
CLAassistant commented May 10, 2016 edited

CLA assistant check
All committers have signed the CLA.

@bep bep commented on an outdated diff May 10, 2016
hugolib/site_test.go
@@ -296,6 +296,42 @@ func TestDraftAndFutureRender(t *testing.T) {
viper.Set("BuildFuture", false)
}
+func FutureExpirationRender(t *testing.T) {
@bep
bep May 10, 2016 Collaborator

I'm pretty sure it has to be named Test* something for it to run.

@bep bep commented on an outdated diff May 10, 2016
commands/hugo.go
@@ -216,6 +217,7 @@ func initHugoBuildCommonFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&cleanDestination, "cleanDestinationDir", false, "Remove files from destination not found in static directories")
cmd.Flags().BoolVarP(&draft, "buildDrafts", "D", false, "include content marked as draft")
cmd.Flags().BoolVarP(&future, "buildFuture", "F", false, "include content with publishdate in the future")
+ cmd.Flags().BoolVarP(&expired, "buildExpired", "E", false, "include content with already expired")
@bep
bep May 10, 2016 Collaborator

Text: "include expired content"

@bep bep commented on an outdated diff May 10, 2016
docs/content/commands/hugo.md
@@ -28,6 +28,7 @@ hugo
-b, --baseURL string hostname (and path) to the root, e.g. http://spf13.com/
-D, --buildDrafts include content marked as draft
-F, --buildFuture include content with publishdate in the future
+ -E, --buildExpired include content already expired
@bep
bep May 10, 2016 Collaborator

All content below /commands is autogenerated so no need to update these manually. Please remove these changes so this PR gets easier to read.

We usually do a ful update before a release.

@bep bep commented on an outdated diff May 10, 2016
docs/content/overview/configuration.md
@@ -87,6 +87,8 @@ Following is a list of Hugo-defined variables that you can configure and their c
buildDrafts: false
# include content with publishdate in the future
buildFuture: false
+ # includ content already expired
@bep
bep May 10, 2016 Collaborator

include

@bep bep commented on an outdated diff May 10, 2016
hugolib/page.go
@@ -486,6 +488,13 @@ func (p *Page) IsFuture() bool {
return true
}
+func (p *Page) IsExpired() bool {
+ if p.ExpiryDate.After(time.Now()) {
@bep
bep May 10, 2016 Collaborator

This method should be a one-liner.

@g3wanghc
Contributor
g3wanghc commented May 10, 2016 edited

@bep What's a good way to make make check more verbose? I'm having trouble finding my formatting errors. 😢

@bep bep commented on the diff May 10, 2016
hugolib/page.go
@@ -467,7 +468,8 @@ func (p *Page) LinkTitle() string {
}
func (p *Page) ShouldBuild() bool {
@bep
bep May 10, 2016 edited Collaborator

This method has gotten complex enough to deserve a refactor.

I would pull out a function (as in not a method) with:

  • buildFuture
  • buildExpired
  • buildDrafts
  • publishDate
  • expiryDate

I.e. no dependency on Page nor Viper, then create a table driven test for that function.

@g3wanghc
g3wanghc May 12, 2016 Contributor

@bep Is what I'm doing correct or does it need more work? 😁

@bep
Collaborator
bep commented May 10, 2016 edited

As to make and gofmt, the best is to actually do gfmting your code. But remember that make uses

gofmt -s (s = simplifies)

so gofmt -s -w thefile.go

@bep bep and 1 other commented on an outdated diff May 10, 2016
hugolib/site_test.go
+ t.Fatalf("Unable to create pages: %s", err)
+ }
+ return s
+ }
+
+ viper.Set("baseurl", "http://auth/bub")
+
+ s := siteSetup()
+
+ if len(s.Pages) > 1 {
+ t.Fatal("Expired content published unexpectedly")
+ }
+
+ if len(s.Pages) < 1 {
+ t.Fatal("Valid content expired unexpectedly")
+ }
@bep
bep May 10, 2016 Collaborator

These two assertions looks a little bit funky. What you want to know is that len(Pages) == 1 and s.Pages[0].title = "doc2"

@g3wanghc
g3wanghc May 10, 2016 Contributor

Hahaha yeah my bad, I will fix that.

@g3wanghc
Contributor

I had to fix my commit messages hehehe. :V

@bep
Collaborator
bep commented May 19, 2016

As to why this hasn't been merged. This looks perfectly fine, very good, and will be merged eventually. I have announced a kind-of freeze period before the 0.16 release, but that seems to drag out.

@moorereason moorereason commented on an outdated diff May 19, 2016
hugolib/page.go
}
}
- return false
+ if !buildExpired {
+ if IsExpiredPage(expiryDate) {
+ return false
+ }
+ }
+ if !(buildDrafts || !Draft) {
+ return false
+ }
+ return true
+}
+
+func IsExpiredPage(expiryDate time.Time) bool {
@moorereason
moorereason May 19, 2016 Collaborator

The IsExpiredPage and IsFuturePage functions have nothing to do with pages, really. They just compare times to time.Now(). They should probably be named something like IsExpiredTime and IsFutureTime.

@moorereason moorereason commented on an outdated diff May 19, 2016
hugolib/page.go
}
}
- return false
+ if !buildExpired {
+ if IsExpiredPage(expiryDate) {
+ return false
+ }
+ }
+ if !(buildDrafts || !Draft) {
+ return false
+ }
+ return true
+}
+
+func IsExpiredPage(expiryDate time.Time) bool {
+ if expiryDate.IsZero() {
@moorereason
moorereason May 19, 2016 Collaborator

Simplify this function and IsFuturePage to just be:

return !expiryDate.IsZero() && !expiryDate.After(time.Now())

Or just get rid of these functions and roll that logic back into AssertShouldBuild.

@moorereason moorereason commented on an outdated diff May 19, 2016
hugolib/page.go
}
}
- return false
+ if !buildExpired {
+ if IsExpiredPage(expiryDate) {
+ return false
+ }
+ }
+ if !(buildDrafts || !Draft) {
@moorereason
moorereason May 19, 2016 Collaborator

We should test for drafts before we test for publish/expire dates.

@g3wanghc
Contributor
@moorereason moorereason commented on an outdated diff May 19, 2016
hugolib/page.go
@@ -468,13 +468,26 @@ func (p *Page) LinkTitle() string {
}
func (p *Page) ShouldBuild() bool {
- if (viper.GetBool("BuildFuture") || p.PublishDate.IsZero() || p.PublishDate.Before(time.Now())) &&
- (viper.GetBool("BuildExpired") || p.ExpiryDate.IsZero() || p.ExpiryDate.After(time.Now())) {
- if viper.GetBool("BuildDrafts") || !p.Draft {
- return true
+ return AssertShouldBuild(viper.GetBool("BuildFuture"), viper.GetBool("BuildExpired"),
+ viper.GetBool("BuildDrafts"), p.Draft, p.PublishDate, p.ExpiryDate)
+}
+
+func AssertShouldBuild(buildFuture bool, buildExpired bool, buildDrafts bool, Draft bool,
+ publishDate time.Time, expiryDate time.Time) bool {
+ if !(buildDrafts || !Draft) {
+ return false
+ }
+ if !buildFuture {
@moorereason
moorereason May 19, 2016 Collaborator

Collapse these if-sets down one more level:

if !buildFuture && !publishDate.IsZero() && publishDate.After(time.Now()) {
@moorereason moorereason and 1 other commented on an outdated diff May 19, 2016
hugolib/page_test.go
+ {true, false, false, false, past, past, false},
+ {true, false, false, false, future, future, true},
+ {true, true, false, false, future, future, true},
+ {false, true, false, false, future, past, false},
+
+ // buildDrafts and draft
+ {true, true, false, true, past, future, false},
+ {true, true, true, true, past, future, true},
+ {true, true, true, true, past, future, true},
+ }
+
+ for _, ps := range publishSettings {
+ s := AssertShouldBuild(ps.buildFuture, ps.buildExpired, ps.buildDrafts, ps.draft,
+ ps.publishDate, ps.expiryDate)
+ if s != ps.out {
+ t.Errorf("AssertShouldBuild unexpected output with prams: %+v", ps)
@moorereason
moorereason May 19, 2016 Collaborator

prams should be params?

@g3wanghc
g3wanghc May 19, 2016 Contributor

Yes, yes it should be 😎

@bep
Collaborator
bep commented Jun 9, 2016

This looks good. I took it for a spin, and the vital parts looks fine, but the list expired lists all of my pages -- I have only one with expiredDate.

@g3wanghc
Contributor

Oops, will fix it. :P

g3wanghc added some commits Jun 13, 2016
@g3wanghc g3wanghc hugolib: Handle unpecified date for IsFuture and IsExpired bfc61ea
@g3wanghc g3wanghc hugolib: futureStats plural
a53615f
@g3wanghc
Contributor

@bep Fixed the list, and a typo.

I've noticed something weird in replacePage on site.go.

func (s *Site) replacePage(page *Page) {
    // will find existing page that matches filepath and remove it
    s.removePage(page)
    s.addPage(page)
}

If the original page wasn't built, then it wouldn't be part of site.Pages.

If you've modified an unpublishedPage, replacePage will be called but removePage wouldn't do anything since it can't find it in site.Pages. addPage will be called and futureCount is increased by 1.

If you added a future publishDate to an existing Page in site.Pages, then removePage will decrease futureCount by -1, and addPage will increate futureCount by 1, resulting in a net gain of 0.

Therefore, Site stats after replacePage is called will always be wrong.

Should we just keep track a list of Pages on Site that aren't being built?

@bep
Collaborator
bep commented Jun 13, 2016

@g3wanghc create a separate issue for the stats counters.

@g3wanghc
Contributor

@bep Done. #2211

@bep
Collaborator
bep commented Jun 14, 2016

Merged, solid work, thanks.

@bep bep closed this Jun 14, 2016
@g3wanghc
Contributor

Awesome!

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