On Mon, Jun 10, 2013 at 9:41 AM, Alan Cabrera <[email protected]> wrote: > On Jun 9, 2013, at 9:40 PM, Greg Stein <[email protected]> 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
