X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/492ea4f58a7499444db59ab843d6a62642bff790..b33efc1d715fd81ed7b83665c998c663cd5db4b4:/doc/todo/calendar_autocreate.mdwn diff --git a/doc/todo/calendar_autocreate.mdwn b/doc/todo/calendar_autocreate.mdwn index 6cb15df47..2a7350b79 100644 --- a/doc/todo/calendar_autocreate.mdwn +++ b/doc/todo/calendar_autocreate.mdwn @@ -14,3 +14,212 @@ won't be offended if you correct stuff you consider awkward): [[!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 copied those options from the [[plugins/tag]] plugin: the +> > `tag_autocreate_commit` option exists and default to 1. +> > +> > It should definitely exists: suppose a calendar page is created and not +> > commited, and later, someone tries to push some changes where a page with +> > the same name has been created. This would result in a conflict. The +> > `calendar_autocreate_commit` prevents this. +> +> > > `tag_autocreate_commit` exists because when tag autocreation +> > > was introduced, they were always in the `$srcdir` and committed. +> > > I changed it so that it was possible to put them in the [[plugins/transient]] +> > > underlay and not commit them. It defaults to 1 to preserve existing +> > > functionality. +> > > +> > > When automatic tag pages (or autoindex pages) are not committed, they +> > > go in the transient underlay, which means they can't cause conflicts: +> > > independent page creation will simply mask them (a page in the +> > > `$srcdir` hides a page of the same name in an underlay). I thought +> > > this implementation did the same when not committing? --[[smcv]] +> +> > > > I did not realize how easy it was to use the [[plugins/transient]] +> > > > plugin! I [[took it into +> > > > account|https://github.com/paternal/ikiwiki/commit/492a22ac75f8b41a427a98c44525b01a6fd181b5]]. +> > > > -- [[Louis|spalax]] +> +> 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. +> +> > [[I agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]] +> +> 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 agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]] +> +> 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. +> +> > [[Done|https://github.com/paternal/ikiwiki/commit/59b46942e01b32138d056381249effbbaf773892]]. +> > I added an option `calendar_fill_gaps` to chose between the two +> > alternatives (since skipping empty months and years would change the +> > default behaviour of this plugin). +> > +> > I think the code is a bit ugly at some places. Perl is not one the the +> > programming languages I am fluent into. Sorry. +> > +> > PS: Good idea, thought. I now have to implement a similar thing for +> > [[plugins/contrib/jscalendar]]. +> +> 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"). +> +> > This is not lost. What ikiwiki-calendar do is simply: build the missing +> > `archive/year/month` pages, and run `ikiwiki -refresh`. With my patch, the +> > `ikiwiki -refresh` includes: +> > +> > - the build of missing `archive/year/month` pages; +> > - highlighting the current day (this was already the case). +> > +> > So one can simply drop the `ikiwiki-calendar ...` for `ikiwiki --refresh +> > ...` in cron to get the same result. +> > +> > I +> > [[tried|https://github.com/paternal/ikiwiki/commit/7a92444e56fe023cea3b074dc5e6b5c4acdb6114]] +> > to make the documentation clearer. +> +> [[!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. +> +> > Oops! It was a mistake. +> > [[Corrected|https://github.com/paternal/ikiwiki/commit/de9842ecc8914e11e73148dae78cd6909b535262]]. +> +> --[[smcv]] +> +> > Thank you for this review. -- [[Louis|spalax]] + +--- + +[[smcv]], can you please go on reviewing this? + +> I don't think I'm really the reviewer you want, since I don't have commit +> access (as you might be able to tell from the number of pending branches +> I have)... but nobody with commit access seems to be available to do +> reviews at the moment, so I'm probably the best you're going to get. +> +> + 0 0 * * * ikiwiki ~/ikiwiki.setup --refresh +> +> I think that should be `ikiwiki --setup ~/ikiwiki.setup --refresh` +> +> The indentation of some of the new code in `IkiWiki/Plugin/calendar.pm` +> is weird. Please use one hard tab (U+0009) per indent step: you seem +> to have used a mixture of one hard tab per indent or two spaces +> per indent, which looks bizarre for anyone whose tab size is not +> 2 spaces. +> +> + return unless $config{calendar_autocreate}; +> +> This is checked in `gencalendaryear` but not in `gencalendarmonth`. +> Shouldn't `gencalendarmonth` do it too? Alternatively, do the check +> in `scan`, which calls `gencalendarmonth` directly. +> +> + my $year = $date[5] + 1900; +> +> You calculate this, but you don't seem to do anything with it? +> +> + if (not exists $changed{$params{year}}) { +> + $changed{$params{year}} = (); +> + } +> + $changed{$params{year}}{$params{month}} = 1; +> +> `$changed{$params{year}}` is a scalar (you can tell because it starts with the +> `$` sigil) but `()` is a list. I think you want `{}` +> (a scalar that is a reference to an empty anonymous hash). +> +> However, that whole `if` block can be omitted, and you can just use +> `$changed{$params{year}}{$params{month}} = 1;`, because Perl will automatically +> create `$changed{$params{year}}` as a reference to an empty hash if necessary, +> in order to put the pair `$params{month} => 1` in it (the term to look +> up if you're curious is "autovivification"). +> +> --[[smcv]]