More robust parsing of SVN client version

Review Request #7182 — Created April 7, 2015 and submitted

Information

RBTools
master
dbd0aac...

Reviewers

A user of SlikSvn reported an error when parsing the svn client
version string.  For SlikSvn the version string is of the form:
  1.6.1-SlikSvn-tag-1.6.1
which is more complicated than the version string produced by
other svn clients where the format is:
  1.6.1

String parsing is updated to extract exactly the first three 
numbers in the version string.  If less than three numbers are
present, which is an unlikely occurrence based on historical 
SVN version numbers (https://subversion.apache.org/docs/release-notes/release-history.html)
then a warning is logged and a default version number of
0.0.0 is assumed.
1) Tested with simple version string produced by stock subversion client in CentOS 7.x.
2) Stepped through code and manually tested with more complicated SlikSvn version string.
3) Ran unit tests.
Description From Last Updated

This should have a leading underscore.

brenniebrennie

This code is getting a bit messy. Can we perhaps add a constant on the class that specifically captures the …

brenniebrennie

Blank line between these. (I know there's stuff violating this right above, but that can be fixed later.)

brenniebrennie

Use double quotes here for the inner string.

brenniebrennie

Col: 48 E231 missing whitespace after ','

reviewbotreviewbot

Col: 50 E231 missing whitespace after ','

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/svn.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/svn.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/clients/svn.py (Diff revision 1)
     
     
     
    Show all issues

    This code is getting a bit messy.

    Can we perhaps add a constant on the class that specifically captures the first three numerical groups and then do something like self.subversion_client_version = tuple(map(int, m.groups)) where m is the result of of doing the regular expression match?

    1. Thanks for the feedback, Barret.

      I'm not opposed to something like this, but I'm not sure I'm regex savvy enough to pull it off in a robust fashion :) I think I could use (\d+)\.(\d+)\.(\d+) to capture the first three decimal components, but it requires three components so something like 1.2 (rather than 1.2.3) doesn't match. I don't know if a two component version is even possible from an SVN client, but obviously they don't all follow the simple three component flavor like I previously thought, so I was trying to be defensive against this in my solution. My approach will work for one to three components. Do you think it is even worth worrying about the number of components in the version string?

    2. I was going to suggest (\d+)\.(\d+)\.(\d+) myself, I didn't consider the case where there are less than 3 version parts. I just assume that SVN will always have three. In that case, we can do (\d+)(?:\.(\d+))*, which will capture all leading dot-separated numbers.

    3. Also make sure to use re.match and not re.find if you go with that idea. I'm not sure which is a more appropriate way to parse it out.

    4. I couldn't get this to work. It seemed to always only capture the final dot-separated number, for example:

      >>> re.match(r'(\d+)(?:\.(\d+))*', '1.6.10.5').groups()
      ('1', '5')
      

      I tinkered with it for a while, but couldn't improve things. I checked the SVN version history (https://subversion.apache.org/docs/release-notes/release-history.html) and ever since 1.0.0 was released in 2004, the version numbers have been triples. I guess it is not possible to know what each individual client implementation would report (a la SlikSvn) but hopefully they would respect and report the full version triple.

      I have a second approach which I'm going to post shortly which requires a version triple for parsing and logs a warning otherwise and sets the version to 0.0.0. See what you think about it, or perhaps you see something I was doing incorrectly above.

    5. So as it turns out, Python's re library doesn't support getting multiple values out of the same capture group so it can't be done in the generic way with just regex.

  3. 
      
gmyers
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/svn.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/clients/svn.py
    
    
  2. rbtools/clients/svn.py (Diff revision 2)
     
     
    Show all issues
    Col: 48
     E231 missing whitespace after ','
    
  3. rbtools/clients/svn.py (Diff revision 2)
     
     
    Show all issues
    Col: 50
     E231 missing whitespace after ','
    
  4. 
      
gmyers
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/svn.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/svn.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/clients/svn.py (Diff revisions 1 - 3)
     
     
    Show all issues

    This should have a leading underscore.

    1. I can change this if you think it is important, but none of the other dozen or so class variables have a leading underscore so I'd suggest leaving if off for consistency.
  3. rbtools/clients/svn.py (Diff revisions 1 - 3)
     
     
     
    Show all issues

    Blank line between these. (I know there's stuff violating this right above, but that can be fixed later.)

  4. rbtools/clients/svn.py (Diff revisions 1 - 3)
     
     
    Show all issues

    Use double quotes here for the inner string.

  5. 
      
gmyers
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/svn.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/svn.py
    
    
  2. 
      
brennie
  1. Ship It!
  2. 
      
gmyers
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.7.x (4db748d)
gmyers
  1. Since this has been submitted, could someone please close issue 3834.  Thanks!
  2.