[[!template id=gitbranch branch=spalax/calendar-autocreate browse="https://github.com/paternal/ikiwiki/tree/calendar-autocreate" author="[[Louis|spalax]]"]]
--[[Louis|spalax]]
+
+> An attempt at a review (although note that I don't have commit access,
+> so my opinion is not final):
+>
+> Should `calendar_autocreate_commit` really default to 1? I would personally
+> expect that any new features that synthesize new pages should not commit
+> them by default - I'd prefer to avoid cluttering git history with generated
+> pages. (Indeed, should the option even exist?)
+>
+> I'd personally do the conditional in gencalendaryear more like:
+>
+> [[!format perl """
+return unless $config{calendar_autocreate};
+"""]]
+>
+> to reduce the indentation depth of the more interesting code.
+>
+> The recursion to generate missing years:
+>
+> [[!format perl """
+if (not exists $wikistate{calendar}{minyear}) {
+ $wikistate{calendar}{minyear} = $year;
+} elsif ($wikistate{calendar}{minyear} > $year) {
+ gencalendaryear($year + 1);
+ $wikistate{calendar}{minyear} -= 1;
+}
+"""]]
+>
+> does seem to be correct on closer examination, but it took me a while
+> to work out that it would actually do the right thing by recursing:
+>
+> * generate 2005
+> * recurse to generate 2006
+> * recurse to generate 2007
+> * recurse to generate 2008
+> * recurse to generate 2009
+> * recurse to try to generate 2010 (no effect)
+> * minyear = minyear - 1 = 2010 - 1 = 2009
+> * minyear = minyear - 1 = 2009 - 1 = 2008
+> * minyear = minyear - 1 = 2008 - 1 = 2007
+> * minyear = minyear - 1 = 2007 - 1 = 2006
+> * minyear = minyear - 1 = 2006 - 1 = 2005
+>
+> I think it might be clearer (as well as less
+> recursion-happy) to use iteration:
+>
+> * generate 2005
+> * recurse to generate 2006
+> * ...
+> * recurse to generate 2009
+> * minyear = 2005
+>
+> something like this:
+>
+> [[!format perl """
+sub gencalendaryear {
+ my $year = shift;
+ my %params = @_;
+ ...
+ # generate this year
+ ...
+ # Filling potential gaps in years [...] years 2006 to 2009.
+ return if $params{norecurse};
+ if (not exists $wikistate{calendar}{minyear}) {
+ $wikistate{calendar}{minyear} = $year;
+ } elsif ($wikistate{calendar}{minyear} > $year) {
+ foreach my $other ($year + 1 .. $wikistate{calendar}{minyear} - 1) {
+ gencalendar($year, norecurse => 1);
+ }
+ $wikistate{calendar}{minyear} = $year;
+ }
+ # ... and the opposite for maxyear
+}
+"""]]
+>
+> I'm not sure about generating missing years at all, though: if the
+> generation is entirely dynamic, and there were no posts at all during
+> a particular year (or month for that matter), shouldn't we just skip
+> the year/month? That seems to be what e.g. Wordpress does.
+>
+> This piece of ikiwiki-calendar functionality is lost:
+>
+> [[!format diff """
+- ... It also refreshes the wiki, updating the calendars to
+-highlight the current day. This command is typically run at midnight from
+-cron.
+"""]]
+>
+> If I understand correctly, the highlight will be on the day at which
+> the wiki was last refreshed, which seems arbitrary and confusing.
+> If ikiwiki-calendar is not used, I'd say there should just not be a
+> highlight for today (although I'm not sure how best to implement that -
+> perhaps a config option representing "I am going to use ikiwiki-calendar").
+>
+> [[!format diff """
+-\[[!template id=plugin name=calendar author="\[[ManojSrivastava]]"]]
+-\[[!tag type/widget]]
+"""]]
+>
+> Why did you remove that? It's useful information about the plugin
+> which I think ought to stay.
+>
+> --[[smcv]]