-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
@@ -296,6 +296,42 @@ func TestDraftAndFutureRender(t *testing.T) { | |||
viper.Set("BuildFuture", false) | |||
} | |||
|
|||
func FutureExpirationRender(t *testing.T) { |
There was a problem hiding this comment.
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.
@bep What's a good way to make |
@@ -467,7 +468,8 @@ func (p *Page) LinkTitle() string { | |||
} | |||
|
|||
func (p *Page) ShouldBuild() bool { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? 😁
As to make and gofmt, the best is to actually do gfmting your code. But remember that make uses
so |
|
||
if len(s.Pages) < 1 { | ||
t.Fatal("Valid content expired unexpectedly") | ||
} |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
I had to fix my commit messages hehehe. :V |
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 { |
There was a problem hiding this comment.
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
.
if !(buildDrafts || !Draft) { | ||
return false | ||
} | ||
if !buildFuture { |
There was a problem hiding this comment.
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()) {
This looks good. I took it for a spin, and the vital parts looks fine, but the |
Oops, will fix it. :P |
@bep Fixed the list, and a typo. I've noticed something weird in
If the original page wasn't built, then it wouldn't be part of If you've modified an If you added a future Therefore, Should we just keep track a list of |
@g3wanghc create a separate issue for the stats counters. |
Merged, solid work, thanks. |
Awesome! |
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. |
First time using goLang and first time contribution. Please be gentle. :V