[this is likely the first time you've seen one of my reviews; while my various reviews' tone may be peremptory, please keep in mind that the intent is feedback, leading to further discussion where you disagree or would like more information; I *very* rarely issue a flat out veto, so unless/until I do, never interpret my reviews as a veto]
On Sun, Jun 9, 2013 at 2:45 AM, <[email protected]> wrote: > Author: adc > Date: Sun Jun 9 06:45:00 2013 > New Revision: 1491149 > > URL: http://svn.apache.org/r1491149 > Log: > Sketch setup of pytonic project organization > > Added: > steve/trunk/bin/ > steve/trunk/bin/votegroup (with props) The existing structure puts all the cmdline tools under cmdline/. I think that makes more sense, as it delineates what scripts are part of the Apache Steve command-line tools. (as opposed to others scripts used within/by the project) > steve/trunk/docs/ > steve/trunk/docs/Makefile > steve/trunk/docs/_build/ > steve/trunk/docs/_static/ > steve/trunk/docs/_templates/ > steve/trunk/docs/conf.py > steve/trunk/docs/index.rst > steve/trunk/docs/make.bat I don't think Apache Steve will ever run on Windows. We depend upon setuid processes. Unless/until that premise changes, then we do not require .BAT files. It might be interesting to just use a Python script rather than a Makefile. We know Python is available. > steve/trunk/requirements.txt Is the unittest framework missing something that we require? Why 'nose' rather than plain unittest? 'mock' is unused. asf-incubator-tools hauls in a HUGE amount of other stuff. At the moment, Steve requires no non-standard dependencies. WIthout some good rationale, then I'd hope that we can continue a clean/simple/minimal install. We certainly don't need keyring services (via incubator tools) to run Apache Steve (for example). > steve/trunk/setup.cfg > steve/trunk/setup.py (with props) > steve/trunk/src/ > steve/trunk/src/steve/ > steve/trunk/src/steve/__init__.py > steve/trunk/src/steve/voters.py When I ported our steve.pm module, I just put them all into one single module. In fact, Jim already collected several modules to incorporate them into that steve.pm. I see no reason to break them out into a package. That is overly-complicated -- steve.py is 157 lines and a dozen simple functions. I don't envision it to grow by much, as Steve has existed in this form for a good while. > steve/trunk/tests/ > steve/trunk/tests/data/ > steve/trunk/tests/data/duplicates.txt > steve/trunk/tests/test_voters.py Again with the dependencies. Why brownie? That line of the test could simply be: assert voters.count('[email protected]') == 2 And a stylistic note for Python coding: I noted that you were using the following form: from setuptools import find_packages This is problematic for the coder/reviewer. Consider the following two chunks of code: ... foo = find_packages('something') ... ... foo = setuptools.find_packages('something') ... The former leads the reader to believe that find_packages is defined within the module. Only when they search, do they find it was defined via import and lives somewhere else. The latter immediately informs the reader that they are using a function from another module. In my experience, it is best to only import modules, rather than symbols from those modules. All references then become MODULE.NAME rather than just NAME. Even worse, in setup.py, I found the following line: from io import open Any code in the rest of the module that has something like: f = open('foo') Will lead the reader to believe that the *builtin* open() function is being used. But that is NOT the case. The above import construction has overwritten the builtin. If the code was written: f = io.open('foo') Then the reader immediately knows what is going on. And the last bit for this review is your @entrypoint decorator. It doesn't do what most people would expect. In many cases, I might write a script like this: #/usr/bin/python #... def main(...): # do something class Support(...) # whatever def helper_func(...): # something class MyException(...): # whatever if __name__ == '__main__': main(...) Your @entrypoint decorators *runs* the main() function immediately. I could not use it within a script structured like above. It would run main() before Support, helper_func, and MyException are defined. IOW, a hidden little implementation detail means that @entrypoint must only be attached to the LAST thing in the script. The __name__ thing is idiomatic. Using a magic decorator with magic requirements is not going to improve the readability of the script. Cheers, -g
