Comment 2 for bug 208606

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jamu Kakar wrote:
| Thank you very much for working on this branch. I'm guessing that it
| isn't complete yet, but I decided to review it anyway. Following are
| my initial set of comments:

That is great, this was the results of my initial hacking.

|
| [1]
|
| +
| +class MergeError(AutoPPAError):
| + """Raised when there are merge conflicts that must be resolved
manually."""
| + print "There are one or more merge conflicts that must be
resolved manually."
|
| There shouldn't be a print statement here. One problem with this is
| that the output generated when the test suite is run will be polluted
| when MergeError's are raised.

Then we need to move the warning in text.py and remove it from there.

|
|
| [2]
|
| +from bzrlib import branch, workingtree
|
| + self.source_branch = branch.Branch.open(self.branch)
| + self.source_working_tree =
workingtree.WorkingTree.open(self.branch)
|
| Please use:
|
| from bzrlib.branch import Branch, bzrdir
| from bzrlib.workingtree import WorkingTree
|
| and
|
| + self.source_branch = Branch.open(self.branch)
| + self.source_working_tree = WorkingTree.open(self.branch)
|

Sure.

| [3]
|
| - self.expect.run("mkdir -p %s" % self.repository,
logfile=self.logfile)
| + try:
| + os.makedirs(self.repository) #Log this somehow?
| + except OSError:
| + pass #Should we abort?
|
| Please use spaces instead of tabs. There are tabs in a few other
| places.
|

I will double check to make sure I am using spaces instead of tabs.

|
| [4]
|
| + try:
| + os.makedirs(self.repository) #Log this somehow?
| + except OSError:
| + pass #Should we abort?
|
| I would be inclined remove the try/except block an let any errors
| bubble up to the outer application layer.

I did this because os.makedirs() raises an OSError if the specified
directory already exists. Suggestions?

|
|
| [5]
|
| + conflicts =
self.source_branch.merge_from_branch(self.release_branch)
| + if conflicts > 0:
|
| This could be simplified to:
|
| conflicts = self.source_branch.merge_from_branch(
| self.release_branch)
| if conflicts:

Easy enough

|
|
| [6]
|
| + raise MergeError
|
| A message should be provided when an exception is raised. I recommend:
|
| raise MergeError(
| "Couldn't merge working branch. Encountered "
| "conflicts: %s" % conflicts)
|
| Even better would be to include data about exactly what the conflicts
| are.

Ahh ok. I have not really read up on the proper way to do exceptions in
python and attempted to infer it from other bits of code.

|
|
| [7]
|
| This change:
|
| - file.close()
| + file.flush()
|
| is followed by this further on:
|
| + file.close()
|
| I'm not getting the logic here. Is there a reason the file can't be
| closed where you've put the call to flush()?

In python, when you call write() on a file, it just writes to the file
object in memory. You must call either flush() or close() to write that
data to the filesystem.

Flush() writes the data to the file system while still alowing the file
to be read. If one closes the file, you must reopen it before reading or
writing to it.

For the commit() call, I am specifying it by reading the file, but
re-reading the code, it makes more sense to just close the file and use
the changelog varriable in the commit. I'll fix that.

In regards to file objects, python's c roots show through.

|
|
| [8]
|
| One of the things the previous pexpect-using logic did was log the
| commands being run. It'd be nice to log messages about the various
| things that are happening with bzrlib, such as when prepared files are
| being reverted, when changes are committed, etc. Maybe bzrlib already
| does this and we need to figure out a way to wire it's logging up to
| AutoPPA's logging?
|

A Vcs system by their very nature log everything they do. Should not be
too difficult to use some of python's built in logging module to keep a
record of each action taken. I'll create a TODO list with these things
on it.

|
| [9]
|
| A number of existing tests are failing. They should be updated to
| test the new logic. I highly recommend you make changes using a
| test-driven development style to ensure that new functionality is
| tested properly and to avoid the situation where you have many
| breaking tests. You can run the tests using trial, the Twisted test
| runner. You'll want to installed python-twisted and run the test
| suite with:
|
| trial autoppa
|
|

Ok cool, I have never worked with self-testing in a codebase before
(self-taught programmer), but I can see the huge advantages of that. I
do the reading and fix the code.

| [10]
|
| There are a few lines longer than 80 characters.

Added to TODO list.

|
|
| I've enjoyed what I've seen so far and am looking forward to seeing
| where you take this!

So do I :)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkfudmkACgkQSyj78chr9d/MwwCgyVLZXefupvet20dGOz1DcmCn
OdsAoKGbeDNbRJZI+lQCs7yfcIC39kp4
=0ccE
-----END PGP SIGNATURE-----