You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@steve.apache.org by Greg Stein <gs...@gmail.com> on 2013/06/10 06:40:21 UTC
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
[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, <ad...@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)
> 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('a@b.com') == 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
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by ke...@apache.org.
>> zc.buildout+pip+setuptools can do that automatically with entry_points even when their sources end in .py.
>
> I'm a Python newbie. I don't understand what that last sentence means. :)
Heh, no worries!
In a Python package's "setup.py", you can list a series of "entry points".
Entry points describe the high-level morphology of your package. Entry points include things like:
- Scripts that should be installed to be run from the command-line (bin)
- System-admin scripts (sbin)
- WSGI web applications
- Twisted services
- Django/Zope/Plone add-ons
- And more!
For example, suppose you had in your setup.py:
setup(
'demo',
...
entry_points={
'console_scripts': [
'vote = demo.cmdline.vote',
'tally = demo.cmdline.tally',
],
'z3c.autoinclude.plugin': [
'target = plone',
]
...
)
If a user installs the package "demo", then she'll get:
- bin/vote (generated from demo/cmdline/vote.py)
- bin/tally (generated from demo/cmdline/tally.py)
- And a Zope auto-include plugin for Plone
Nice, huh?
--k
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 10, 2013, at 11:23 AM, kelly@apache.org wrote:
>> Do we really have to have .py at the end of our command line tools? (yuck) :)
>
> Personally, I'd prefer not.
>
> zc.buildout+pip+setuptools can do that automatically with entry_points even when their sources end in .py.
I'm a Python newbie. I don't understand what that last sentence means. :)
Regards,
Alan
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by ke...@apache.org.
> Do we really have to have .py at the end of our command line tools? (yuck) :)
Personally, I'd prefer not.
zc.buildout+pip+setuptools can do that automatically with entry_points even when their sources end in .py.
--k
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jun 12, 2013 at 8:37 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
> On Jun 11, 2013, at 10:31 PM, Greg Stein <gs...@gmail.com> wrote:
>> On Wed, Jun 12, 2013 at 12:03 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
>>> ...
>>> Why wouldn't we use os.setuid()?
>>
>> Only root can call os.setuid(), and voter certainly never runs as root :-)
>>
>> The compiled copies of wrapsuid.c will use filesystem's setuid bit to
>> switch users from $whoever to 'voter' (or whatever is configured).
>> Thus, the Python interpreter will execute under the voter (effective)
>> UID.
>
> I really hate not having pure Python dists. But, if there's no other way I guess we're stuck.
Totally agreed -- my preference, too. But yeah: we're stuck.
At least on the current model. We *could* create a pure server-based
option. Voters/clients authenticate using their key. No setuid would
be necessary since voters would not be using a shell, or have bare
access to the vote tallies. Mark this as "future feature work" :-)
(we'll always have the ssh-based option, for organizations whose
security model is ssh-based like the ASF, and that always means
mixed-source distributions)
Cheers,
-g
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 11, 2013, at 10:31 PM, Greg Stein <gs...@gmail.com> wrote:
> On Wed, Jun 12, 2013 at 12:03 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
>> ...
>> Why wouldn't we use os.setuid()?
>
> Only root can call os.setuid(), and voter certainly never runs as root :-)
>
> The compiled copies of wrapsuid.c will use filesystem's setuid bit to
> switch users from $whoever to 'voter' (or whatever is configured).
> Thus, the Python interpreter will execute under the voter (effective)
> UID.
I really hate not having pure Python dists. But, if there's no other way I guess we're stuck.
Regards,
Alan
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jun 12, 2013 at 12:03 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
>...
> Why wouldn't we use os.setuid()?
Only root can call os.setuid(), and voter certainly never runs as root :-)
The compiled copies of wrapsuid.c will use filesystem's setuid bit to
switch users from $whoever to 'voter' (or whatever is configured).
Thus, the Python interpreter will execute under the voter (effective)
UID.
Cheers,
-g
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jun 12, 2013 at 12:04 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
>...
> Just read Kelly's email. Ignore my last question. :D
You mean *Sean* Kelly?
(and from else-where, it is *Chris* Mattmann, rather than "Matt")
:-P
Cheers,
-g
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 11, 2013, at 9:03 PM, Alan Cabrera <li...@toolazydogs.com> wrote:
>
> On Jun 10, 2013, at 11:38 AM, Greg Stein <gs...@gmail.com> wrote:
>
>>> ...
>>> Do we really have to have .py at the end of our command line tools? (yuck) :)
>>
>> Look at cmdline/.
>>
>> All the scripts end in .pl. The Makefile builds equivalents based on
>> wrapsuid.c that omit the suffix. The *real* invocation (which uses
>> setuid) will not have the suffix. The module can/should so that we can
>> import the module for testing, or for other needs.
>
> Why wouldn't we use os.setuid()?
>
> Can setup.py be coded in some way to drop the .py extensions?
Just read Kelly's email. Ignore my last question. :D
Regards,
Alan
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 10, 2013, at 11:38 AM, Greg Stein <gs...@gmail.com> wrote:
>> ...
>> Do we really have to have .py at the end of our command line tools? (yuck) :)
>
> Look at cmdline/.
>
> All the scripts end in .pl. The Makefile builds equivalents based on
> wrapsuid.c that omit the suffix. The *real* invocation (which uses
> setuid) will not have the suffix. The module can/should so that we can
> import the module for testing, or for other needs.
Why wouldn't we use os.setuid()?
Can setup.py be coded in some way to drop the .py extensions?
Regards,
Alan
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 10, 2013 at 2:21 PM, Alan Cabrera <li...@toolazydogs.com> wrote:
>
> On Jun 10, 2013, at 10:30 AM, Greg Stein <gs...@gmail.com> wrote:
>...
>> Look in cmdline! There are six cmdline tools. We probably need to
>> shift some of the monitoring tools in there, too.
>>
>> I don't know what would go into bin/.
>
> The scripts that are in cmdline seem to be six Apache Steve cmdline tools. You mention "other scripts used within/by the project". What are those scripts?
I have no idea. We have none. IOW, I would say bin/ isn't needed right
now. We have nothing to put into it (assuming the continued use of
cmdline/).
> With that said, bin/ seems to be the Python standard to put command line tools; I use boto as an example. Is this not the case?
Apache Steve will install two sets of things:
* a set of command-line tools
* a web-based interface to those tools
cmdline/ is the first batch, and www/ is the second.
We have some stuff under monitoring/ that needs to be shifted around.
And some other miscellaneous files to get organized.
>>>>> steve/trunk/requirements.txt
>>>>
>>>> Is the unittest framework missing something that we require? Why
>>>> 'nose' rather than plain unit test?
>>>
>>> I love nose.
>>
>> What does it provide that we'll need?
"that we'll need".
>
> Advantages:
>
> You can write test functions, and not forced to write test classes.
> Automatic tests discovery and collecting, you do not have to build test suites manually.
> Plugin support
> Very useful standard plugins (coverage, output capture, drop into debugger on errors, doctests support, profiler)
> Test tagging and easy selection of test sets based on tags.
> Parallel testing
> Flexible fixtures support
> Test generators
> It could very well be that plain unit test supports the above and it's my ignorance that has led me to my opinions.
How many of those will we need?
We have about 2000 lines of script. Much of that is actually
boilerplate text or html. Do we need "plugin support" in our unit
tests?
Or do we need things like:
assert "a@b.com" in voters
?
My concern is that some of these dependencies are what I'd see for a
large, complicated system. Apache Steve isn't that. I don't even see
how/where it would go into something like that.
>...
>> Nope. It isn't a package' it's just a simple library module. It is a
>> dozen small helper functions.
>>
>> Unless/until that dramatically grows (towards what?), then I think we
>> should choose simple.
>
> I'm sure that I'm being dense. How does that answer my question about moving your code to steve/trunk/src/steve/__init__.py? Should we not put all our library code in steve/trunk/src/steve?
"All" is just steve.py. 156 lines.
We could have src/steve.py. I didn't think that we would have any
other source, so I put it into lib/, much like Python modules are
installed into /usr/lib or /usr/local/lib or ....
So again: I'm not presupposing anything, and I just put it into lib/.
We can always rearrange if/when we find things get more complicated.
>...
> Do we really have to have .py at the end of our command line tools? (yuck) :)
Sigh.
Look at cmdline/, please.
All the scripts end in .pl. The Makefile builds equivalents based on
wrapsuid.c that omit the suffix. The *real* invocation (which uses
setuid) will not have the suffix. The module can/should so that we can
import the module for testing, or for other needs.
Cheers,
-g
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 10, 2013, at 10:30 AM, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Jun 10, 2013 at 9:41 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
>> On Jun 9, 2013, at 9:40 PM, Greg Stein <gs...@gmail.com> wrote:
>> ...
>>>> 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?
>
> Look in cmdline! There are six cmdline tools. We probably need to
> shift some of the monitoring tools in there, too.
>
> I don't know what would go into bin/.
The scripts that are in cmdline seem to be six Apache Steve cmdline tools. You mention "other scripts used within/by the project". What are those scripts?
With that said, bin/ seems to be the Python standard to put command line tools; I use boto as an example. Is this not the case?
>>>> steve/trunk/requirements.txt
>>>
>>> Is the unittest framework missing something that we require? Why
>>> 'nose' rather than plain unit test?
>>
>> I love nose.
>
> What does it provide that we'll need?
Advantages:
You can write test functions, and not forced to write test classes.
Automatic tests discovery and collecting, you do not have to build test suites manually.
Plugin support
Very useful standard plugins (coverage, output capture, drop into debugger on errors, doctests support, profiler)
Test tagging and easy selection of test sets based on tags.
Parallel testing
Flexible fixtures support
Test generators
It could very well be that plain unit test supports the above and it's my ignorance that has led me to my opinions.
>>>> 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?
>
> Nope. It isn't a package' it's just a simple library module. It is a
> dozen small helper functions.
>
> Unless/until that dramatically grows (towards what?), then I think we
> should choose simple.
I'm sure that I'm being dense. How does that answer my question about moving your code to steve/trunk/src/steve/__init__.py? Should we not put all our library code in steve/trunk/src/steve?
>> ...
>>> 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.
>
> Well... with a .py extension (like we've done in cmdline/*.pl), then
> it is also an importable module. As you know, that's the whole reason
> for the __name__ construct: to differentiate between invocation and
> importing. As a result, you may want to structure/order the file
> differently if you're building a dual-purpose module. (eg. self-test
> if run as a script, but normally be a module)
Sounds like mixing concerns, but I won't belabor the issue.
Do we really have to have .py at the end of our command line tools? (yuck) :)
Regards,
Alan
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/
docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jun 10, 2013 at 9:41 AM, Alan Cabrera <li...@toolazydogs.com> wrote:
> On Jun 9, 2013, at 9:40 PM, Greg Stein <gs...@gmail.com> wrote:
>...
>>> 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?
Look in cmdline! There are six cmdline tools. We probably need to
shift some of the monitoring tools in there, too.
I don't know what would go into bin/.
>...
>> 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.
*shrug* ... just a thought.
>>> steve/trunk/requirements.txt
>>
>> Is the unittest framework missing something that we require? Why
>> 'nose' rather than plain unit test?
>
> I love nose.
What does it provide that we'll need?
>...
>> asf-incubator-tools hauls in a HUGE amount of other stuff.
>
> Yes, it does. I'm using it as a utility.
I don't see any value yet. Just a dependency that we didn't have before.
We should use argparse. We should use email.utils.parseaddr().
>> 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.
I just threw that out as one example of something hauled in by that
dependency. Our voting is based on LDAP/uid, rather than the keyring.
While I agree there is a tradeoff, I don't see the "win" on this just yet.
>>> 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?
Nope. It isn't a package' it's just a simple library module. It is a
dozen small helper functions.
Unless/until that dramatically grows (towards what?), then I think we
should choose simple.
>...
>> 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.
Well... with a .py extension (like we've done in cmdline/*.pl), then
it is also an importable module. As you know, that's the whole reason
for the __name__ construct: to differentiate between invocation and
importing. As a result, you may want to structure/order the file
differently if you're building a dual-purpose module. (eg. self-test
if run as a script, but normally be a module)
>> 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.
And that value is?
> But, if you feel strongly I don't mind too much about removing it.
Well... most people won't understand what @entrypoint means/does, or
its implications. Every Python programmer understands the name/main
idiom.
Cheers,
-g
Re: svn commit: r1491149 - in /steve/trunk: ./ bin/ docs/ docs/_build/ docs/_static/ docs/_templates/ src/ src/steve/ tests/ tests/data/
Posted by Alan Cabrera <li...@toolazydogs.com>.
On Jun 9, 2013, at 9:40 PM, Greg Stein <gs...@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, <ad...@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