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

Support an "expiration" date #726 #2137

Closed
wants to merge 15 commits into from
Closed

Support an "expiration" date #726 #2137

wants to merge 15 commits into from

Conversation

g3wanghc
Copy link
Contributor

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

@CLAassistant
Copy link

CLAassistant commented May 10, 2016

CLA assistant check
All committers have signed the CLA.

@@ -296,6 +296,42 @@ func TestDraftAndFutureRender(t *testing.T) {
viper.Set("BuildFuture", false)
}

func FutureExpirationRender(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@g3wanghc
Copy link
Contributor Author

g3wanghc commented May 10, 2016

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

@@ -467,7 +468,8 @@ func (p *Page) LinkTitle() string {
}

func (p *Page) ShouldBuild() bool {
Copy link
Member

@bep bep May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bep
Copy link
Member

bep commented May 10, 2016

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


if len(s.Pages) < 1 {
t.Fatal("Valid content expired unexpectedly")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha yeah my bad, I will fix that.

@g3wanghc
Copy link
Contributor Author

I had to fix my commit messages hehehe. :V

@bep
Copy link
Member

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.

return true
}

func IsExpiredPage(expiryDate time.Time) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@g3wanghc
Copy link
Contributor Author

@moorereason 👌

if !(buildDrafts || !Draft) {
return false
}
if !buildFuture {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collapse these if-sets down one more level:

if !buildFuture && !publishDate.IsZero() && publishDate.After(time.Now()) {

@bep
Copy link
Member

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
Copy link
Contributor Author

Oops, will fix it. :P

@g3wanghc
Copy link
Contributor Author

@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
Copy link
Member

bep commented Jun 13, 2016

@g3wanghc create a separate issue for the stats counters.

@g3wanghc
Copy link
Contributor Author

@bep Done. #2211

@bep
Copy link
Member

bep commented Jun 14, 2016

Merged, solid work, thanks.

@bep bep closed this Jun 14, 2016
@g3wanghc
Copy link
Contributor Author

Awesome!

@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 Feb 22, 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