Re: [PATCH] svnlook.py: Make it usable as a library

Wow. Thanks for the review and committing the stuff so quick.

About dates, I need to check if the behaviour changed for myself.
Here is the diff commented - my original version first.

> $ diff -bu3 svnlook.py svnlook.py\?revision\=1541558 -p
...
> -__version__ = '2012.05.24'
> -

Because svnlook.py is used in hook scripts, it is usually copied
out of revision control. Version is useful to track new changes.

> @@ -76,7 +74,7 @@ class SVNLook(object):
> self.txn_ptr = None
> if rev is None:
> rev = fs.youngest_rev(self.fs_ptr)
> - self.rev = int(rev)
> + self.rev = rev

I believe that this is critical for some API calls and arithmetic that
self.rev is not a string.

> def cmd_date(self):
> - # duplicate original svnlook format, which is
> - # 2010-02-08 21:53:15 +0200 (Mon, 08 Feb 2010)
> secs = self.get_date(unixtime=True)
> - # convert to tuple, detect time zone and format
> - stamp = time.localtime(secs)
> - isdst = stamp.tm_isdst
> - utcoffset = -(time.altzone if (time.daylight and isdst) else time.timezone) // 60
> -
> - suffix = "%+03d%02d" % (utcoffset // 60, abs(utcoffset) % 60)
> - outstr = time.strftime('%Y-%m-%d %H:%M:%S ', stamp) + suffix
> - outstr += time.strftime(' (%a, %d %b %Y)', stamp)
> - print(outstr)
> + if secs is None:
> + print("")
> + else:
> + # assume secs in local TZ, convert to tuple, and format
> + ### we don't really know the TZ, do we?
> + print(time.strftime('%Y-%m-%d %H:%M', time.localtime(secs)))

I believe that svnlook.py output was different than of svnlook, but I may
mistake. Anyway, this change seems ok.

> def get_date(self, unixtime=False):
> """return commit timestamp in RFC 3339 format (2010-02-08T20:37:25.195000Z)
> if unixtime is True, return unix timestamp
> - return None if date is not set (txn properties)
> + return None for a txn, or if date property is not set

What are other cases when date may be empty?

Anyway, LGTM without any more fixes.

--
anatoly t.
On Wed, Nov 13, 2013 at 7:13 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> (I sound so grumpy. Let me try again.)
>
> Thank you, Anatoly, for bothering to contribute your changes back to the community.
>
> Your main changes look good and well commented. I like them and I've committed them.
>
> It always makes it easier and thus more likely that someone will commit your patch if you supply a log message even for a simple change, but in this case I wrote one. (Afterwards, I saw that in GitHub you have a series of log messages for the changes that made up this patch,  but even if I'd seen them earlier I would still have needed to combine them into a single Subversion-style message.)
>
> It's also best to avoid unrelated changes in the same patch. In this case, I stripped out the changes that seemed to be unrelated. Apologies if I missed something important.
>
> Thanks again.
> - Julian
>
>
> I (Julian Foad) wrote:
>
>> Hi, anatoly.
>>
>> Your patch contains:
>>
>>   * make usable as a library by adding getter methods;
>>
>>   * when printing a date, use a different formatting;
>>
>>   * minor changes:
>>     adding a source code version label;
>>     setting self.rev to youngest rev even if a txn-id is provided;
>>     s/youngest/latest/ in the help.
>>
>>   * no log message.
>>
>> The main changes (adding getter methods) look good. I have tested the changes by
>> hand, written a log message, and committed:
>> <http://svn.apache.org/r1541558>.
>>
>> The change to the display of dates is a separate issue. I don't really care
>> what format it displays, but this code has a problem: it now displays a date
>> even when there is no "svn:date" property.
>>
>> Are any of the minor changes important or useful?
>>
>> Thanks.
> [...]
>>>> https://github.com/apache/subversion/pull/1/files
>>>>
>>>> Please, CC.