Re: [PATCH 4/6] git-p4: Decode byte strings before printing

Joel Holdsworth [off-list ref] writes:

Signed-off-by: Joel Holdsworth <redacted>
---
 git-p4.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Is the use of strings with {} placeholders and their .format() method
integral part of "decoding byte strings before printing", or it is just
a new/better/improved/subjectively-preferred/whatever style?

If the latter, such a change should be separated into its own step,
or at least needs to be mentioned and justified in the proposed log
message.

Lack of explanation on "why" is shared among all these patches, it
seems, so I won't repeat, but the patches need to explain why to
their readers.

On 09.12.2021 14:47, Junio C Hamano wrote:

Joel Holdsworth [off-list ref] writes:

quoted

Signed-off-by: Joel Holdsworth <redacted>
---
 git-p4.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Is the use of strings with {} placeholders and their .format() method
integral part of "decoding byte strings before printing", or it is just
a new/better/improved/subjectively-preferred/whatever style?

If the new minimum python version will be 3.6 or above I'd vote for using 
f-Strings instead of .format() which I think are more readable and are also 
supposed to be faster.

So:
sys.stdout.write(f'\r{file_path} --> {rel_path} ({size/1024/1024} MB)\n')

instead of one of these:
sys.stdout.write('\r%s --> %s (%i MB)\n' % (file_path, relPath, size/1024/1024))
sys.stdout.write('\r{} --> {} ({} MB)\n'.format(file_path.decode(), relPath, 
size/1024/1024))

quoted

diff --git a/git-p4.py b/git-p4.py
index b5d4fc1176..b5945a0306 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -2917,7 +2917,8 @@ def streamOneP4File(self, file, contents):
                 size = int(self.stream_file['fileSize'])
             else:
                 size = 0 # deleted files don't get a fileSize apparently
-            sys.stdout.write('\r%s --> %s (%i MB)\n' % (file_path, relPath, size/1024/1024))
+            sys.stdout.write('\r{} --> {} ({} MB)\n'.format(
+                file_path.decode(), relPath, size/1024/1024))
             sys.stdout.flush()

         (type_base, type_mods) = split_p4_type(file["type"])

@@ -3061,7 +3062,8 @@ def streamP4FilesCb(self, marshalled):
             size = int(self.stream_file["fileSize"])
             if size > 0:
                 progress = 100*self.stream_file['streamContentSize']/size
-                sys.stdout.write('\r%s %d%% (%i MB)' % (self.stream_file['depotFile'], progress, int(size/1024/1024)))
+                sys.stdout.write('\r{} {}% ({} MB)'.format(
+                    self.stream_file['depotFile'].decode(), progress, int(size/1024/1024)))
                 sys.stdout.flush()

         self.stream_have_file_info = True

@@ -3803,7 +3805,7 @@ def closeStreams(self):
             return
         self.gitStream.close()
         if self.importProcess.wait() != 0:
-            die("fast-import failed: %s" % self.gitError.read())
+            die("fast-import failed: {}".format(self.gitError.read().decode()))
         self.gitOutput.close()
         self.gitError.close()
         self.gitStream = None

Is the use of strings with {} placeholders and their .format() method integral
part of "decoding byte strings before printing", or it is just a
new/better/improved/subjectively-preferred/whatever style?

If the latter, such a change should be separated into its own step, or at least
needs to be mentioned and justified in the proposed log message.

As I mentioned in my other message, I would like to invest some time into tidying and modernising the script - as well as fixing bugs and improving behaviour. If I submit patches that only make subjective style improvements, are these likely to be accepted?

Lack of explanation on "why" is shared among all these patches, it seems, so I
won't repeat, but the patches need to explain why to their readers.

Fair enough. I will resubmit.

If the new minimum python version will be 3.6 or above I'd vote for using f-
Strings instead of .format() which I think are more readable and are also
supposed to be faster.

Time passes so fast - I would prefer to use f-strings, but I didn't realise that they were universally available yet. They're still a "new thing" as far as I'm concerned.

I would prefer f-strings, I just used the str.format() method as a middle-ground.

So:
sys.stdout.write(f'\r{file_path} --> {rel_path} ({size/1024/1024} MB)\n')

By the way, I have a patch coming soon that can print the size in human readable units: b, kb, Mb, Gb etc. rather than always converting it to Mb.