doc/todo/calendar_autocreate.mdwn

   1 Here is a patch that makes [[ikiwiki-calendar]] almost useless.
   2
   3 It adds some options, the main one being `calendar_autocreate`, which is
   4 similar to the `tag_autocreate` option of the [[tag|plugins/tag]]: it create
   5 archive pages when needed.
   6
   7 The documentation is updated as well (but as a non-native English speaker, I
   8 won't be offended if you correct stuff you consider awkward):
   9
  10 - [[plugin|https://github.com/paternal/ikiwiki/blob/calendar-autocreate/doc/plugins/calendar.mdwn]]
  11 - [[directive|https://github.com/paternal/ikiwiki/blob/calendar-autocreate/doc/ikiwiki/directive/calendar.mdwn]]
  12
  13 [[!tag patch]]
  14 [[!template  id=gitbranch branch=spalax/calendar-autocreate browse="https://github.com/paternal/ikiwiki/tree/calendar-autocreate" author="[[Louis|spalax]]"]]
  15
  16 --[[Louis|spalax]]
  17
  18 > An attempt at a review (although note that I don't have commit access,
  19 > so my opinion is not final):
  20 >
  21 > Should `calendar_autocreate_commit` really default to 1? I would personally
  22 > expect that any new features that synthesize new pages should not commit
  23 > them by default - I'd prefer to avoid cluttering git history with generated
  24 > pages. (Indeed, should the option even exist?)
  25 >
  26 > > I copied those options from the [[plugins/tag]] plugin: the
  27 > > `tag_autocreate_commit` option exists and default to 1.
  28 > >
  29 > > It should definitely exists: suppose a calendar page is created and not
  30 > > commited, and later, someone tries to push some changes where a page with
  31 > > the same name has been created. This would result in a conflict. The
  32 > > `calendar_autocreate_commit` prevents this.
  33 >
  34 > > > `tag_autocreate_commit` exists because when tag autocreation
  35 > > > was introduced, they were always in the `$srcdir` and committed.
  36 > > > I changed it so that it was possible to put them in the [[plugins/transient]]
  37 > > > underlay and not commit them. It defaults to 1 to preserve existing
  38 > > > functionality.
  39 > > >
  40 > > > When automatic tag pages (or autoindex pages) are not committed, they
  41 > > > go in the transient underlay, which means they can't cause conflicts:
  42 > > > independent page creation will simply mask them (a page in the
  43 > > > `$srcdir` hides a page of the same name in an underlay). I thought
  44 > > > this implementation did the same when not committing? --[[smcv]]
  45 >
  46 > > > > I did not realize how easy it was to use the [[plugins/transient]]
  47 > > > > plugin! I [[took it into
  48 > > > > account|https://github.com/paternal/ikiwiki/commit/492a22ac75f8b41a427a98c44525b01a6fd181b5]].
  49 > > > > -- [[Louis|spalax]]
  50 >
  51 > I'd personally do the conditional in gencalendaryear more like:
  52 >
  53 > [[!format perl """
  54 return unless $config{calendar_autocreate};
  55 """]]
  56 >
  57 > to reduce the indentation depth of the more interesting code.
  58 >
  59 > > [[I agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]]
  60 >
  61 > The recursion to generate missing years:
  62 >
  63 > [[!format perl """
  64 if (not exists $wikistate{calendar}{minyear}) {
  65         $wikistate{calendar}{minyear} = $year;
  66 } elsif ($wikistate{calendar}{minyear} > $year) {
  67         gencalendaryear($year + 1);
  68         $wikistate{calendar}{minyear} -= 1;
  69 }
  70 """]]
  71 >
  72 > does seem to be correct on closer examination, but it took me a while
  73 > to work out that it would actually do the right thing by recursing:
  74 >
  75 > * generate 2005
  76 >   * recurse to generate 2006
  77 >     * recurse to generate 2007
  78 >       * recurse to generate 2008
  79 >         * recurse to generate 2009
  80 >           * recurse to try to generate 2010 (no effect)
  81 >         * minyear = minyear - 1 = 2010 - 1 = 2009
  82 >       * minyear = minyear - 1 = 2009 - 1 = 2008
  83 >     * minyear = minyear - 1 = 2008 - 1 = 2007
  84 >   * minyear = minyear - 1 = 2007 - 1 = 2006
  85 > * minyear = minyear - 1 = 2006 - 1 = 2005
  86 >
  87 > I think it might be clearer (as well as less
  88 > recursion-happy) to use iteration:
  89 >
  90 > * generate 2005
  91 >   * recurse to generate 2006
  92 >   * ...
  93 >   * recurse to generate 2009
  94 > * minyear = 2005
  95 >
  96 > something like this:
  97 >
  98 > [[!format perl """
  99 sub gencalendaryear {
 100         my $year = shift;
 101         my %params = @_;
 102         ...
 103         # generate this year
 104         ...
 105         # Filling potential gaps in years [...] years 2006 to 2009.
 106         return if $params{norecurse};
 107         if (not exists $wikistate{calendar}{minyear}) {
 108                 $wikistate{calendar}{minyear} = $year;
 109         } elsif ($wikistate{calendar}{minyear} > $year) {
 110                 foreach my $other ($year + 1 .. $wikistate{calendar}{minyear} - 1) {
 111                         gencalendar($year, norecurse => 1);
 112                 }
 113                 $wikistate{calendar}{minyear} = $year;
 114         }
 115         # ... and the opposite for maxyear
 116 }
 117 """]]
 118 >
 119 >
 120 > > [[I agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]]
 121 >
 122 > I'm not sure about generating missing years at all, though: if the
 123 > generation is entirely dynamic, and there were no posts at all during
 124 > a particular year (or month for that matter), shouldn't we just skip
 125 > the year/month? That seems to be what e.g. Wordpress does.
 126 >
 127 > > [[Done|https://github.com/paternal/ikiwiki/commit/59b46942e01b32138d056381249effbbaf773892]].
 128 > > I added an option `calendar_fill_gaps` to chose between the two
 129 > > alternatives (since skipping empty months and years would change the
 130 > > default behaviour of this plugin).
 131 > >
 132 > > I think the code is a bit ugly at some places. Perl is not one the the
 133 > > programming languages I am fluent into. Sorry.
 134 > >
 135 > > PS: Good idea, thought. I now have to implement a similar thing for
 136 > > [[plugins/contrib/jscalendar]].
 137 >
 138 > This piece of ikiwiki-calendar functionality is lost:
 139 >
 140 > [[!format diff """
 141 - ... It also refreshes the wiki, updating the calendars to
 142 -highlight the current day. This command is typically run at midnight from
 143 -cron.
 144 """]]
 145 >
 146 > If I understand correctly, the highlight will be on the day at which
 147 > the wiki was last refreshed, which seems arbitrary and confusing.
 148 > If ikiwiki-calendar is not used, I'd say there should just not be a
 149 > highlight for today (although I'm not sure how best to implement that -
 150 > perhaps a config option representing "I am going to use ikiwiki-calendar").
 151 >
 152 > > This is not lost. What ikiwiki-calendar do is simply: build the missing
 153 > > `archive/year/month` pages, and run `ikiwiki -refresh`. With my patch, the
 154 > > `ikiwiki -refresh` includes:
 155 > >
 156 > > - the build of missing `archive/year/month` pages;
 157 > > - highlighting the current day (this was already the case).
 158 > >
 159 > > So one can simply drop the `ikiwiki-calendar ...` for `ikiwiki --refresh
 160 > > ...` in cron to get the same result.
 161 > >
 162 > > I
 163 > > [[tried|https://github.com/paternal/ikiwiki/commit/7a92444e56fe023cea3b074dc5e6b5c4acdb6114]]
 164 > > to make the documentation clearer.
 165 >
 166 > [[!format diff """
 167 -\[[!template id=plugin name=calendar author="\[[ManojSrivastava]]"]]
 168 -\[[!tag type/widget]]
 169 """]]
 170 >
 171 > Why did you remove that? It's useful information about the plugin
 172 > which I think ought to stay.
 173 >
 174 > > Oops! It was a mistake.
 175 > > [[Corrected|https://github.com/paternal/ikiwiki/commit/de9842ecc8914e11e73148dae78cd6909b535262]].
 176 >
 177 > --[[smcv]]
 178 >
 179 > > Thank you for this review. -- [[Louis|spalax]]
 180
 181 ---
 182
 183 [[smcv]], can you please go on reviewing this?