1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
|
Here is a patch that makes [[ikiwiki-calendar]] almost useless.
It adds some options, the main one being `calendar_autocreate`, which is
similar to the `tag_autocreate` option of the [[tag|plugins/tag]]: it create
archive pages when needed.
The documentation is updated as well (but as a non-native English speaker, I
won't be offended if you correct stuff you consider awkward):
- [[plugin|https://github.com/paternal/ikiwiki/blob/calendar-autocreate/doc/plugins/calendar.mdwn]]
- [[directive|https://github.com/paternal/ikiwiki/blob/calendar-autocreate/doc/ikiwiki/directive/calendar.mdwn]]
[[!tag patch]]
[[!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]]
|