[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#659498: RFS: solarpowerlog -- photovoltaic data logging

Am Freitag, den 17.02.2012, 12:48 +0100 schrieb Benoît Knecht:

Sorry for the late reply, spare time was quite spare the last days...

> Hi Tobias,
> As promised, here's my review of your package:
>   - In debian/solarpowerlog.default, you define /etc/solarpowerlog as
>     the default RUNDIR; what files is your program expecting to find
>     there, HTML templates? If so, installing some default templates in
>     /usr/share/solarpowerlog and pointing your program to that directory
>     seems like a better option.

The templates were in an earlier release at /etc/solarpowerlog, but I
moved them to /usr/share/docs/solarpowerlog/examples after I got
received some critism about the usage of the similie timeline widgets,
which are (if they are) only very hard to package. 
So I won't install any default templates except as the said examples,
but I will remove the reference to the rundir and hint in the installed
example configuration file to use absolute paths.  

>     You also note there that start-stop-daemon is taking care of running
>     solarpowerlog in the background, but according to
>     start-stop-daemon(8), "this is a last resort, and is only meant for
>     programs that either make no sense forking on their own, or where
>     it's not feasible to add the code for them to do this themselves";
>     since your program can put itself in the background, you should
>     rather use that possibility. This way, it will also make sense
>     checking the exit status of start-stop-daemon in do_start in the
>     init script, because if you use its '-b' option, it can't determine
>     if the process failed to execute.

Yes, the daemon support of solarpowerlog is a mixture of barely tested,
incomplete, limited, you name it.. At least up to version 0.22, as I
worked on this with the last commit to my development branch. In other
words, 0.23, which I plan to release soon, will get rid of the hacks you
mentined above.  

>     And it'd be best to wrap lines at 80 columns.
>   - Running uscan tells me:
>       Processing watchfile line for package solarpowerlog...
>       Newest version on remote site is 0.21a, local version is 0.22
>       solarpowerlog: remote site does not even have current version

I had an typo ("_" instead of "-") when I uploaded the latest tarball to
sf.net. Should work now. 

>   - I think your short description should read "photovoltaic data
>     logger", as in "solarpowerlog is a ..."
>     And in the long description, you wrote "photo-voltaic"; I think you
>     should stick to the former spelling.
>     The second paragraph of the long description is missing punctuation.

Fixed in my debian branch.

>   - In the solarpowerlog(1) man page, the SEE ALSO section appears to be
>     a subsection of OPTIONS.
>     Please consider removing the AUTHOR section (see man-pages(7) for an
>     explanation why).
>     In DESCRIPTION, I would start with "solarpowerlog tracks and
>     logs..."; it's not just its purpose, it's what it actually does,
>     right? Same for the second sentence.
>     In OPTIONS, you start with "These programs"; why the plural?

The easy things are already fixed, the complete rewording will take some
time ;-) 
Especially as I added several command line options (thanks to the
improved daemon support) I will have to rework those manpage. Its a
pitty that I like coding more than wording, but it is defintly on the

> Cheers,
> -- 
> Benoît Knecht

Many thanks for the feedback,


Reply to: