Code review comment for lp:~vivian-vdesmedt/ipython/synchronize-editor

Revision history for this message
Fernando Perez (fdo.perez) wrote :

Mmh, I was getting ready to merge and push, but then I went and looked at the code, and I see the whole thing is written in camelCase():

http://bazaar.launchpad.net/%7Evivian-vdesmedt/ipython/synchronize-editor/annotate/1007?file_id=ipy_synchronize_with-20080612032938-lxufpyi2plzu3mlc-1

Why is that the case? For all new code, we are really trying to stick to proper PEP-8 within reason, and I don't want to violate that unless there's a good motive for it.

I also don't see any docstrings in any of the functions at all.

Vivian, I realize that this may be a bit painful, but as you can see, we're now making an honest effort at building a much higher quality codebase for IPython, so that all code that goes in is reviewed, documented and tested. And we are all living by the same rules (see recent reviews on other branches, and all the work that has gone into having testing machinery for ipython itself).

With bzr and launchpad, it's relatively easy for you to track the trunk (simply keep merging from trunk into your working branch and pushing back upstream frequently), so that your branch stays up to date, as you improve it. But I really think that we can't merge this code in until it's properly documented and it matches the naming conventions of the rest of the project.

I realize it's a bit frustrating at this point, but hopefully it's clear to you that these rules are there to ensure that as the whole project grows in a very healthy fashion. We're getting more new code coming in than any one of us can handle individually, and the only way we won't crumble under the weight is to enforce that all this new code is compliant with the rest of the project, documented and automatically tested.

« Back to merge proposal