[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] v2 Fixed gen_win.py to display Serf library version#

From: Edmund Wong <ed_at_kdtc.net>
Date: Mon, 14 Sep 2009 08:49:13 +0800

Daniel Shahaf wrote:
> Edmund Wong wrote on Mon, 14 Sep 2009 at 01:36 +0800:
>> [[[
>>
>> Fixed gen_win.py such that it checks for the presence of the
>> Serf library (which it does prior to this patch) and also
>> outputs the library version (which is what this patch does).
>>
>> * build/generator/gen_win.py
>> (_get_serf_version): New function to return the serf library
>> version in a 3-tuple format.
>
> The '(' should be indented two spaces.

I'll get this fixed.

>
>> (_find_serf): Added a version check. If the 3-tuple version
>> structure has -1 in the first index, the serf library is
>> not built; otherwise, the library is set to be built. Then
>> a message is displayed informing the user of the results.
>>
>
> IMO, the part about -1 and 3-tuple is a low-enough implementation detail
> to not belong in the log message. Just say what the added code thus
> (extract the version, message if successful, skip building(?) otherwise).

I see. I'll modify that part of the log message.

>> +
>> + if self.serf_path and os.path.exists(self.serf_path):
>> + if (os.path.exists(os.path.join(self.serf_path, 'serf.h'))):
>> + fp = open(os.path.join(self.serf_path, 'serf.h'))
>> + txt = fp.read()
>> + maj_t = re.compile(r'SERF_MAJOR_VERSION (\d+)')
>> + min_t = re.compile(r'SERF_MINOR_VERSION (\d+)')
>> + patch_t = re.compile(r'SERF_PATCH_VERSION (\d+)')
>> + ver_maj_re = maj_t.search(txt)
>> + ver_min_re = min_t.search(txt)
>> + ver_patch_re = patch_t.search(txt)
>
> This reads the entire file to the variable 'txt'. It would be better to
> read it incrementally (e.g., one line at a time).

I just realized i didn't even close the file.

>
> (And perhaps relax the regexes by allowing any number of whitespace
> there? This is almost a matter of personal taste, though.)

That is a point. Regexes isn't my strong point. So would the
following suffice?

   maj_t = re.compile(r'SERF_MAJOR_VERSION(\s+)(\d+)')

Edmund

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2394394
Received on 2009-09-14 02:56:13 CEST

This is an archived mail posted to the Subversion Dev mailing list.