<div>On 11/6/06, John Arbash Meinel <<a href="mailto:john@arbash-meinel.com">john@arbash-meinel.com</a>> wrote:<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I highly recommend learning to write tests along with code. I'm not 100% dedicated to test-driven yet, but test-as-you-go is really nice. Test-driven can be really good too, I just don't always use it. (Basically, to know your code works, you have to run it anyway, so you might as well put that in a real test, and then in the future you will be aware if that test ever fails).</blockquote><div> I completely aggree with you. I would have liked started with writing tests, but i did the patch more for myself in the first place. I will write the tests as soon as possible.  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> 2) We generally submit patches as text files (not .zip, etc) because that way we can review them inside an email, rather than downloading it, unzipping it, opening another editor, and the copying code chunks back to do comments.</blockquote><div> will comply next time  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">3) If you read HACKING, you can see some of our style guides, but the important ones are:    1) Use spaces, not tabs to indent    2) Wrap lines at 79 characters</blockquote><div> whooops. i thought i  did a '%s/\t/    /g'  before  commit .... </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> 4) You changed the parameter 'to_name' to 'to_dir'. I agree that it is more descriptive, but because in python any parameter can be accessed by its' name, we have to maintain API compatibility. (eg move(from_paths=[], to_name='foo')) However, we can do this: def move(self, from_paths, to_dir=None, after=False, **kwargs): ... if to_dir is None:   to_name = kwargs.get('to_name', None)   if to_name is None:     raise ? ('You must supply a target directory')   else:     symbol_versioning.warn('The parameter to_name was deprecated'                            ' in version 0.13. Use to_dir instead',                            DeprecationWarning)     to_dir = to_name I'm not sure what the exact exception should be. Python would raise an TypeError if you didn't supply a required argument. Which seems good enough here. It is a programmer error, not a runtime error. (We generally avoid raising builtin errors, but in this case it makes sense)</blockquote><div> i will fix it. </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> 5) This section: +            tuple = {} +            tuple['from_rel']  = from_rel +            tuple['from_id']   = from_id +            tuple['from_tail'] = from_tail +            tuple['from_parent_id'] = from_parent_id +            tuple['to_rel']    = to_rel +            tuple['to_tail']   = from_tail   # from_tail == to_tail +            tuple['to_parent_id'] = to_dir_id +            from_to_tuples.append(tuple) First, you call it a 'tuple' but it is a dictionary. (Python has a tuple type (1, 2, 3)) Second, PEP8 requests that you don't add whitespace before your '=' sign. I understand why you like it, but for consistency (and to prevent people going back and forth changing this sort of thing) we've decided to follow PEP8's recommendations.</blockquote><div> What is PEP8 ? </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Third, I think you have enough stuff going on here that a real object would be better than a dictionary. We won't be creating so many of them that a dictionary performance would matter. And I think it ends up being clearer. And if you want to keep the naming, you still can, it just becomes: class _RenameEntry(object):   def __init__(self, from_rel, from_id, from_tail, from_parent_id,                to_rel, to_tail, to_parent_id):      self.from_rel = from_rel ...      self.only_change_inv = None ...    entry = _RenamEntry(from_rel=from_rel,                        from_id=from_id,                        from_tail=from_tail,                        ...                       )</blockquote> <div> seems like  i have coded too much Ruby and not enough Python lately :-) Dictionaries instead of ValueObjects are overused in Ruby. Will change it. </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> 6) All of the mutter() statements should be change: +        mutter("  from_id   {%s}" % from_id) +        mutter("  from_rel  %r" % from_rel) +        mutter("  to_rel    %r" % to_rel) +        mutter("  to_dir    %r" % to_dir) +        mutter("  to_dir_id {%s}" % to_dir_id) + This wasn't your bug, they were already incorrect. But at the least, these should be: +        mutter("  from_id   {%s}", from_id) +        mutter("  from_rel  %r", from_rel) +        mutter("  to_rel    %r", to_rel) +        mutter("  to_dir    %r", to_dir) +        mutter("  to_dir_id {%s}", to_dir_id) + Otherwise if there was a '%' symbol in one of the filenames, it would cause mutter() to puke, because mutter() also does a string expansion with the arguments. (Mutter should be updated to not puke, but this is still incorrect). Further, I prefer to see stuff like this in a single call, so something like: mutter('rename one:  {%s}\n'        '  from rel:  %r\n'        '  to_rel:    %r\n'        '  to_dir:    %r\n'        '  to_dir_id: {%s}\n',        from_id, from_rel, to_rel, to_dir, to_dir_id)</blockquote><div> Will change it, too. </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> 7) I also discovered that your bundle used 'dos' line endings (CRLF). This is a bug with how we interact with Windows (win defaults to setting stdin/stdout/etc as text mode, when we expect binary mode). But it means that you should use 'bzr bundle --output=filename'</blockquote><div> Can you please update the 'Giving Back' document accordingly? </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I hope this isn't more feedback than you wanted. I think the general flow of what you did was good, just some tweaks to make it fit with our coding style, etc.</blockquote><div> No. That is exactly what i wanted. We are talking about consistency here. That is vital to every software project that has to be maintained for the next two or three decades (and that is the plan, isn't it ;-)) Thank you for taking the time. Ciao,   Steffen </div> </div>