Re: [PATCH] change-svn-wc-format.py

On Sat, 28 Mar 2009 21:37:36 +0000, Stefan Sperling wrote
> > Index: change-svn-wc-format.py
> > ===================================================================
> > --- change-svn-wc-format.py (revision 36814)
> > +++ change-svn-wc-format.py (working copy)
> > @@ -95,7 +95,18 @@
> > sys.stderr.write("%s, skipping\n" % e)
> > sys.stderr.flush()
>
> At this point, we might want to skip the creation of the format
> file if parsing the entries file failed.
>
> >
> > + format = Format(os.path.join(dirname, get_adm_dir(), "format"))
> > if self.verbosity:
> > + print("Updating file '%s'" % format.path)
> > + try:
> > + format.write_format(format_nbr, self.verbosity)
> > + except UnrecognizedWCFormatException, e:
>
> Maybe I'm missing something, but under what conditions will
> format.write_format() raise an UnrecognizedWCFormatException?
> It looks like this exception will only be raised by the
> write_format() method in the Entries class.
>
> So I think we can get rid of the try-except clause around
> format.write_format(format_nbr, self.verbosity)
> and the following four lines:
>
> > + if self.error_on_unrecognized:
> > + raise
> > + sys.stderr.write("%s, skipping\n" % e)
> > + sys.stderr.flush()
> > +
>

Right. I'll fix this.

> We can save a line and a variable here by doing:
>
> if (len(str(format_nbr))) < 2:
> format_string = '%02d'
> else:
> format_string = '%01d'
>
> and getting rid of this line:
>
> > + format_string = frmt_val + 'd'
>

Fixed.
 
> > + # Overwrite all bytes of the format number.
> > + if os.path.exists(self.path):
> > + format = open(self.path,"r")
> > + os.chmod(self.path, 0600)
> > + format_line = format.readline()
> > + format_line.rstrip()
> > + format_nbr_old = int(format_line)
> > + format.close()
> > + format = open(self.path, "w")
> > + format.write(format_string % format_nbr)
> > + if verbosity >= 2:

Regarding this block, I noticed a slight 'uselessness' in that
I get the old value and then I don't do anything to it. Would
the comparison between the old and new number make any sense
right now? If I simply write the new value(which has already
been checked anyway) to the file, would that lessen the code?

I was going to change the above to:

  format_line = format.readline().rstrip()

or even more ambitious:
  format_nbr_old = int(format.readline().rstrip())

but maybe that's a little too reckless?

> > + print("format file has been updated.")
> > + else:
> > + if verbosity >= 1:
> > + print("Format file does not exist. Creating it now.")
>
> Here we could use either lower case ("format file ..."),
> or simply print the filename:
>
> print("%s does not exist, creating it." % self.path)
>
> > + format = open(self.path, "w")
> > + os.chmod(self.path,0600)
> > + format.write(format_string % format_nbr)
> > + if verbosity >= 1:
> > + print("Format file has been created.")
>
> The above print statement is somewhat redundant.
> We've already told the user we were going to create the file,
> and there's nothing much that can go wrong between then and now.
> Except e.g. write permission problems, but then we'll probably
> get an exception we don't catch anyway, which the user will end
> up seeing, right?

I understand. I'll fix that right away.

--
"Pax tecum"
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1464515