short_rev and display_rev should always return a string

[Ryan J Ollos]

Discussed in #12179, the API should document that short_rev and display_rev return a string, and the Subversion implementation should be modified to return a string rather than an integer. Since short_rev and display_rev call normalize_rev, it may be that the latter only needs to be modified.

[Ryan J Ollos]

Proposed change in attachment:t12182.diff​.

[Jun Omae]

I tried the patch and got 4 failures.

FAIL: test_repos_display_rev (tracopt.versioncontrol.svn.tests.svn_fs.SubversionRepositoryNormalTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tracopt/versioncontrol/svn/tests/svn_fs.py", line 140, in test_repos_display_rev
    self.assertEqual(str(HEAD), self.repos.display_rev(None))
AssertionError: '30' != 30

Probably, the following would solve it.

trac/versioncontrol/api.py

@@ -1019 +1019 @@
 
         :raise NoSuchChangeset: If the given `rev` isn't found.
         """
-        norm_rev = self.normalize_rev(rev)
-        return str(norm_rev) if rev is not None else norm_rev
+        rev = self.normalize_rev(rev)
+        return str(rev) if rev is not None else rev
 
     def display_rev(self, rev):
         """Return a string representation of a revision in the repos for
@@ -1031 +1031 @@
 
         :raise NoSuchChangeset: If the given `rev` isn't found.
         """
-        norm_rev = self.normalize_rev(rev)
-        return str(norm_rev) if rev is not None else norm_rev
+        rev = self.normalize_rev(rev)
+        return str(rev) if rev is not None else rev
 
     @abstractmethod
     def get_changes(self, old_path, old_rev, new_path, new_rev,

[Ryan J Ollos]

Thanks for review. Committed to trunk in [14273].