8e6a1a0d0c399dcc459492213862a470779e1dbd
[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 > > > `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?
184
185 > I don't think I'm really the reviewer you want, since I don't have commit
186 > access (as you might be able to tell from the number of pending branches
187 > I have)... but nobody with commit access seems to be available to do
188 > reviews at the moment, so I'm probably the best you're going to get.
189 >
190 >     +    0 0 * * * ikiwiki ~/ikiwiki.setup --refresh
191 >
192 > I think that should be `ikiwiki --setup ~/ikiwiki.setup --refresh`
193 >
194 > The indentation of some of the new code in `IkiWiki/Plugin/calendar.pm`
195 > is weird. Please use one hard tab (U+0009) per indent step: you seem
196 > to have used a mixture of one hard tab per indent or two spaces
197 > per indent, which looks bizarre for anyone whose tab size is not
198 > 2 spaces.
199 >
200 >     + return unless $config{calendar_autocreate};
201 >
202 > This is checked in `gencalendaryear` but not in `gencalendarmonth`.
203 > Shouldn't `gencalendarmonth` do it too? Alternatively, do the check
204 > in `scan`, which calls `gencalendarmonth` directly.
205 >
206 >     +         my $year  = $date[5] + 1900;
207 >
208 > You calculate this, but you don't seem to do anything with it?
209 >
210 >     +  if (not exists $changed{$params{year}}) {
211 >     +    $changed{$params{year}} = ();
212 >     +  }
213 >     +  $changed{$params{year}}{$params{month}} = 1;
214 >
215 > `$changed{$params{year}}` is a scalar (you can tell because it starts with the
216 > `$` sigil) but `()` is a list. I think you want `{}`
217 > (a scalar that is a reference to an empty anonymous hash).
218 >
219 > However, that whole `if` block can be omitted, and you can just use
220 > `$changed{$params{year}}{$params{month}} = 1;`, because Perl will automatically
221 > create `$changed{$params{year}}` as a reference to an empty hash, in order to
222 > put the pair `$params{month} => 1` in it (the term to look
223 > up if you're curious is "autovivification").
224 >
225 > --[[smcv]]