Answer to smcv
[ikiwiki.git] / 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 > I'd personally do the conditional in gencalendaryear more like:
35 >
36 > [[!format perl """
37 return unless $config{calendar_autocreate};
38 """]]
39 >
40 > to reduce the indentation depth of the more interesting code.
41 >
42 > > [[I agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]]
43 >
44 > The recursion to generate missing years:
45 >
46 > [[!format perl """
47 if (not exists $wikistate{calendar}{minyear}) {
48         $wikistate{calendar}{minyear} = $year;
49 } elsif ($wikistate{calendar}{minyear} > $year) {
50         gencalendaryear($year + 1);
51         $wikistate{calendar}{minyear} -= 1;
52 }
53 """]]
54 >
55 > does seem to be correct on closer examination, but it took me a while
56 > to work out that it would actually do the right thing by recursing:
57 >
58 > * generate 2005
59 >   * recurse to generate 2006
60 >     * recurse to generate 2007
61 >       * recurse to generate 2008
62 >         * recurse to generate 2009
63 >           * recurse to try to generate 2010 (no effect)
64 >         * minyear = minyear - 1 = 2010 - 1 = 2009
65 >       * minyear = minyear - 1 = 2009 - 1 = 2008
66 >     * minyear = minyear - 1 = 2008 - 1 = 2007
67 >   * minyear = minyear - 1 = 2007 - 1 = 2006
68 > * minyear = minyear - 1 = 2006 - 1 = 2005
69 >
70 > I think it might be clearer (as well as less
71 > recursion-happy) to use iteration:
72 >
73 > * generate 2005
74 >   * recurse to generate 2006
75 >   * ...
76 >   * recurse to generate 2009
77 > * minyear = 2005
78 >
79 > something like this:
80 >
81 > [[!format perl """
82 sub gencalendaryear {
83         my $year = shift;
84         my %params = @_;
85         ...
86         # generate this year
87         ...
88         # Filling potential gaps in years [...] years 2006 to 2009.
89         return if $params{norecurse};
90         if (not exists $wikistate{calendar}{minyear}) {
91                 $wikistate{calendar}{minyear} = $year;
92         } elsif ($wikistate{calendar}{minyear} > $year) {
93                 foreach my $other ($year + 1 .. $wikistate{calendar}{minyear} - 1) {
94                         gencalendar($year, norecurse => 1);
95                 }
96                 $wikistate{calendar}{minyear} = $year;
97         }
98         # ... and the opposite for maxyear
99 }
100 """]]
101 >
102 >
103 > > [[I agree|https://github.com/paternal/ikiwiki/commit/7f18c1ce48630507b744fa56b83999e8ca684606]]
104 >
105 > I'm not sure about generating missing years at all, though: if the
106 > generation is entirely dynamic, and there were no posts at all during
107 > a particular year (or month for that matter), shouldn't we just skip
108 > the year/month? That seems to be what e.g. Wordpress does.
109 >
110 > > [[Done|https://github.com/paternal/ikiwiki/commit/59b46942e01b32138d056381249effbbaf773892]].
111 > > I added an option `calendar_fill_gaps` to chose between the two
112 > > alternatives (since skipping empty months and years would change the
113 > > default behaviour of this plugin).
114 > >
115 > > I think the code is a bit ugly at some places. Perl is not one the the
116 > > programming languages I am fluent into. Sorry.
117 > >
118 > > PS: Good idea, thought. I now have to implement a similar thing for
119 > > [[plugins/contrib/jscalendar]].
120 >
121 > This piece of ikiwiki-calendar functionality is lost:
122 >
123 > [[!format diff """
124 - ... It also refreshes the wiki, updating the calendars to
125 -highlight the current day. This command is typically run at midnight from
126 -cron.
127 """]]
128 >
129 > If I understand correctly, the highlight will be on the day at which
130 > the wiki was last refreshed, which seems arbitrary and confusing.
131 > If ikiwiki-calendar is not used, I'd say there should just not be a
132 > highlight for today (although I'm not sure how best to implement that -
133 > perhaps a config option representing "I am going to use ikiwiki-calendar").
134 >
135 > > This is not lost. What ikiwiki-calendar do is simply: build the missing
136 > > `archive/year/month` pages, and run `ikiwiki -refresh`. With my patch, the
137 > > `ikiwiki -refresh` includes:
138 > >
139 > > - the build of missing `archive/year/month` pages;
140 > > - highlighting the current day (this was already the case).
141 > >
142 > > So one can simply drop the `ikiwiki-calendar ...` for `ikiwiki --refresh
143 > > ...` in cron to get the same result.
144 > >
145 > > I
146 > > [[tried|https://github.com/paternal/ikiwiki/commit/7a92444e56fe023cea3b074dc5e6b5c4acdb6114]]
147 > > to make the documentation clearer.
148 >
149 > [[!format diff """
150 -\[[!template id=plugin name=calendar author="\[[ManojSrivastava]]"]]
151 -\[[!tag type/widget]]
152 """]]
153 >
154 > Why did you remove that? It's useful information about the plugin
155 > which I think ought to stay.
156 >
157 > > Oops! It was a mistake.
158 > > [[Corrected|https://github.com/paternal/ikiwiki/commit/de9842ecc8914e11e73148dae78cd6909b535262]].
159 >
160 > --[[smcv]]
161 >
162 > > Thank you for this review. -- [[Louis|spalax]]