| Message ID | 20211209201029.136886-5-jholdsworth@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Transition git-p4.py to support Python 3 only | expand |
Joel Holdsworth <jholdsworth@nvidia.com> writes: > Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com> > --- > 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. > 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
On 09.12.2021 14:47, Junio C Hamano wrote: >Joel Holdsworth <jholdsworth@nvidia.com> writes: > >> Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com> >> --- >> 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)) >> 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.
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
Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com> --- git-p4.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)