On Jun 9, 2013, at 9:40 PM, Greg Stein <gst...@gmail.com> wrote:
> [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, <a...@apache.org> 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)
I'm just following the convention that other Python projects, e.g. boto, and
we, at my work, use.
With that said, what other scripts are we talking about?
>> 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.
Ok. I will remove that file.
> It might be interesting to just use a Python script rather than a
> Makefile. We know Python is available.
Yeah, this is what Sphinx coughs up. I think we should leave well enough alone
here since other Sphinx fans will be familiar with things when then go into
this directory.
>> steve/trunk/requirements.txt
>
> Is the unittest framework missing something that we require? Why
> 'nose' rather than plain unit test?
I love nose.
> 'mock' is unused.
Yeah, I know that I will be using it but am not using it at this time. Easy
enough to remove and put it back when I do.
> asf-incubator-tools hauls in a HUGE amount of other stuff.
Yes, it does. I'm using it as a utility.
> 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).
I think we can agree that there's a tradeoff. We will be using the keyring
when people use command line tools to vote.
>> 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.
I, and I think Matt, have a larger vision. I foresee the package becoming a
junk drawer very soon. But I do agree, you've caught pre-mature packaging
there.
With that said, do you agree that your code in lib should move to
steve/trunk/src/steve/__init__.py?
>> 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('a...@b.com') == 2
Ok.
> 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.
Good idea. I'll prefix "external" functions.
> 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(...)
It's a shell script. Why bother with the above conditional? Just wondering.
> 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.
I feel the value that you receive outweighs the __name__/main() idiom.
But, if you feel strongly I don't mind too much about removing it.
Regards,
Alan
