Patchwork D7504: py3: replace %s by %r on binary format string when needed

login
register
mail settings
Submitter phabricator
Date Nov. 22, 2019, 12:05 p.m.
Message ID <differential-rev-PHID-DREV-iviivzuptdjxym5zcmrc-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43439/
State Superseded
Headers show

Comments

phabricator - Nov. 22, 2019, 12:05 p.m.
matclab created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  For some error, mercurial displays the type of a variable. However the
  formatting string used was "%s" which is not compatible with `type` and lead
  to an excpetion `TypeError: %b requires a bytes-like object, or an object that
  implements __bytes__, not 'type'`.
  
  Per pep-046, we replace "%s" with "%r" which stay compatible with python2 and
  python3.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7504

AFFECTED FILES
  contrib/perf.py
  mercurial/bundlerepo.py
  mercurial/localrepo.py
  mercurial/scmutil.py
  mercurial/utils/cborutil.py

CHANGE DETAILS




To: matclab, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 22, 2019, 1:04 p.m.
This revision now requires changes to proceed.
dlax added a comment.
dlax requested changes to this revision.


  nit: the actual PEP is pep-0461 (https://www.python.org/dev/peps/pep-0461/)

INLINE COMMENTS

> localrepo.py:1571
> +                    b"unsupported changeid '%r' of type %r"
>                      % (changeid, pycompat.sysstr(type(changeid)))
>                  )

The first `%s` was correct I think because `changeid` can be a bytes.

For the second one, if `%r` is the way to go (I'm not sure), maybe we can drop `pycompat.sysstr()`?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7504/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7504

To: matclab, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
phabricator - Nov. 22, 2019, 1:26 p.m.
matclab added a comment.
matclab abandoned this revision.




INLINE COMMENTS

> dlax wrote in localrepo.py:1571
> The first `%s` was correct I think because `changeid` can be a bytes.
> 
> For the second one, if `%r` is the way to go (I'm not sure), maybe we can drop `pycompat.sysstr()`?

I'm really dumb… 
I encountered the problem in mercurial 5.2 and did the change in mercurial devel without checking that the problem still occurred. I guess that `pycompat.sysstr()` already solves the problem. 
In my opinion `%r` is shorter and cleaner, but I  don't think we need to bother.

Sorry for the noise. I will close the request.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7504/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7504

To: matclab, #hg-reviewers, dlax
Cc: dlax, mercurial-devel

Patch

diff --git a/mercurial/utils/cborutil.py b/mercurial/utils/cborutil.py
--- a/mercurial/utils/cborutil.py
+++ b/mercurial/utils/cborutil.py
@@ -241,7 +241,7 @@ 
             break
 
     if not fn:
-        raise ValueError(b'do not know how to encode %s' % type(v))
+        raise ValueError(b'do not know how to encode %r' % type(v))
 
     return fn(v)
 
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -597,7 +597,7 @@ 
     """
     if not isinstance(symbol, bytes):
         msg = (
-            b"symbol (%s of type %s) was not a string, did you mean "
+            b"symbol (%r of type %r) was not a string, did you mean "
             b"repo[symbol]?" % (symbol, type(symbol))
         )
         raise error.ProgrammingError(msg)
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1567,7 +1567,7 @@ 
                 rev = self.changelog.rev(node)
             else:
                 raise error.ProgrammingError(
-                    b"unsupported changeid '%s' of type %s"
+                    b"unsupported changeid '%r' of type %r"
                     % (changeid, pycompat.sysstr(type(changeid)))
                 )
 
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -295,7 +295,7 @@ 
             self._cgunpacker = bundle
         else:
             raise error.Abort(
-                _(b'bundle type %s cannot be read') % type(bundle)
+                _(b'bundle type %r cannot be read') % type(bundle)
             )
 
         # dict with the mapping 'filename' -> position in the changegroup.
diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -1058,7 +1058,7 @@ 
         elif isinstance(bundle, streamclone.streamcloneapplier):
             raise error.Abort(b'stream clone bundles not supported')
         else:
-            raise error.Abort(b'unhandled bundle type: %s' % type(bundle))
+            raise error.Abort(b'unhandled bundle type: %r' % type(bundle))
 
     for fn, title in benches:
         timer, fm = gettimer(ui, opts)