Critic my module

Devyn Collier Johnson devyncjohnson at gmail.com
Sat Jul 27 15:19:54 CEST 2013


On 07/26/2013 10:48 PM, Steven D'Aprano wrote:
> As requested, some constructive criticism of your module.
>
> On Thu, 25 Jul 2013 09:24:30 -0400, Devyn Collier Johnson wrote:
>
>> #!/usr/bin/python3
>> #Made by Devyn Collier Johnson, NCLA, Linux+, LPIC-1, DCTS
> What's NCLA, Linux+, LPIC-1, DCTS? Do these mean anything? Are we
> supposed to know what they mean?
>
> "Made by" has no legal significance. You probably want:
>
> Copyright © 2013 Devyn Collier Johnson.
>
>
>> #Made using the Geany IDE
> Nobody gives a monkey's toss what editor you used to type up the module.
> You might as well mention the brand of monitor you used, or whether the
> keyboard is Dvorak or Qwerty.
>
>
>> #LGPLv3
> You can't just drop in a mention of "LGPLv3" and expect it to mean
> anything. You actually have to obey the licence yourself, and that
> includes *actually including the licence in your work*. (You're
> technically in violation of the licence at the moment, however since the
> only person whose copyright you are infringing is yourself, it doesn't
> matter. However anyone else using your code is at risk.)
>
> http://www.gnu.org/licenses/gpl-howto.html
>
> In the case of the LGPL, you have to include the text of *both* the GPL
> and the LGPL, not just one.
>
>
>
>> import re, sys, subprocess, platform
>> def grep(regex,textf):
>> #Sample Command: grep.grep("^x",dir()) #Syntax:
>> boash.grep(regexp_string,list_of_strings_to_search)
> Comments using # are only of use to people reading the source code. If
> you want comments to be available at the interactive prompt, you should
> write them as doc strings:
>
> def grep(regex, textf):
>      """This string is a docstring.
>
>      Sample command: ...
>      Blah blah blah
>      """
>
> Then, at the interactive prompt, the user can say:
>
> help(boash.grep)
>
> to read the docstring.
>
>
>> 	version = '0.2a'
> That's quite useless, since it is a local variable invisible outside of
> the function.
>
> Also, why would you bother giving every individual function a version
> number? That's rather pointless. The user cannot pick and choose function
> A with version number 0.6 and function B with version number 0.7 if the
> module provides versions 0.7 of both.
>
>
>> 	expr = re.compile(regex)
>> 	match = re.findall(expr, textf)
>> 	if match != None:
>> 		print(match)
> When comparing with None, it is preferred to use "is" and "is not" rather
> than equality tests.
>
>
>> def ls():
>> 	version = '0.3'
>> 	print(subprocess.getoutput('ls'))
>> def dir():
>> 	version = '0.3'
>> 	print(subprocess.getoutput('dir'))
> A blank line or two between functions does wonders for readability. There
> is no prize for conserving newlines.
>
> You might like to read PEP 8, the Python style guide. It is optional, but
> still makes a very good guide.
>
> http://www.python.org/dev/peps/pep-0008/
>
>
>> def bash(*arg):
>> 	version = '0.3'
>> 	print(subprocess.getoutput(arg))
>> def shell(*arg):
>> 	version = '0.3'
>> 	print(subprocess.getoutput(arg))
> bash is not a synonym for "shell". "The shell" might be sh, csh, bash, or
> any one of many other shells, all of which are slightly (or not so
> slightly) different.
>
>
>> def clear_bash_history():
>> 	version = '0.3'
>> 	print(subprocess.getoutput('history -c'))
> [...]
>
> Do you really need ten aliases for 'history -c'?
>
> If you want to define aliases for a function, don't recreate the entire
> function ten times. Start with defining the function once, then:
>
> clear_bash_hist = clear_hist = clear_history = clear_bash_history
>
> etc. But really, having ten names for the one function just confuses
> people, who then wonder what subtle difference there is between
> delete_history and clear_history.
>
>> def firefox():
>> 	version = '0.3'
>> 	print(subprocess.Popen('(firefox &)'))
> Is Firefox really so important that it needs a dedicated command?
>
> What about Debian users? Doesn't Iceweasel get a command?
>
>
>> def xterm():
>> 	version = '0.3'
>> 	print(subprocess.Popen('(xterm &)'))
> Surely the user already has an xterm open, if they are running this
> interactively? Why not just use your xterm's "new window" or "new tab"
> command?
>
>
> [...delete more trivial calls to subprocess...]
>
>> def repeat_cmd():
>> 	version = '0.3'
>> 	print(subprocess.Popen('!!'))
> [... delete two exact copies of this function...]
>
>> def ejcd():
>> 	version = '0.3'
>> 	print(subprocess.Popen('eject cdrom1'))
> [... delete FOURTEEN exact copies of this function...]
>
> Really? Is anyone going to type "eject_disc_tray" instead of "eject"?
>
>
> I think that will do.
>
> This doesn't really do anything except define a large number of trivial
> wrappers to commands already available in the shell. Emphasis on the
> *trivial* -- with the exception of the grep wrapper, which is all of four
> lines (ignoring the useless internal version number), every single one of
> these wrapper functions is a one-liner.[1] In other words, you're not
> adding any value to the shell commands by wrapping them in Python. There
> are plenty of big, complex shell commands that take a plethora of options
> and could do with some useful Python wrappers, like wget. But you haven't
> done them.
>
> Nor have you added extra security, or even extra convenience. You've done
> nothing that couldn't be done using the shell "alias" command, except in
> Python where the syntax is less convenient (e.g. "ls" in the shell,
> versus "ls()" in Python).
>
>
>
>
> [1] I think every newbie programmer goes through a stage of pointlessly
> writing one-liner wrappers to every second function they see. I know I
> did. The difference is, before the Internet, nobody did it publicly.
>
>

About the aliases, I have tried setting pwd() as an alias for 
"os.getcwd()", but I cannot type "pwd()" and get the desired output. 
Instead, I must type "pwd". I tested this in Guake running Python3.3.

 >>> os.getcwd()
'/home/collier'
 >>> pwd = os.getcwd()
 >>> pwd()
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
TypeError: 'str' object is not callable
 >>> pwd
'/home/collier'
 >>> pwd() = os.getcwd()
   File "<stdin>", line 1
SyntaxError: can't assign to function call


How could I make pwd() work?

Mahalo,

DCJ



More information about the Python-list mailing list