X-Git-Url: http://git.vanrenterghem.biz/git.ikiwiki.info.git/blobdiff_plain/2c18086a74ead49dddf6a50e56faf3cb7fdb9f0c..b33efc1d715fd81ed7b83665c998c663cd5db4b4:/doc/todo/calendar_autocreate.mdwn?ds=inline

diff --git a/doc/todo/calendar_autocreate.mdwn b/doc/todo/calendar_autocreate.mdwn
index 26466bca6..2a7350b79 100644
--- a/doc/todo/calendar_autocreate.mdwn
+++ b/doc/todo/calendar_autocreate.mdwn
@@ -23,6 +23,31 @@ won't be offended if you correct stuff you consider awkward):
 > 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 """
@@ -31,6 +56,8 @@ 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 """
@@ -89,11 +116,25 @@ sub gencalendaryear {
 }
 """]]
 >
+>
+> > [[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 """
@@ -108,6 +149,20 @@ sub gencalendaryear {
 > 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]]
@@ -116,4 +171,55 @@ sub gencalendaryear {
 > 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]]