Path: csiph.com!aioe.org!news.stack.nl!newsfeed.xs4all.nl!newsfeed3.news.xs4all.nl!xs4all!post.news.xs4all.nl!not-for-mail
Date: Sat, 27 Jul 2013 08:56:10 -0400
From: Devyn Collier Johnson <devyncjohnson@gmail.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130623 Thunderbird/17.0.7
MIME-Version: 1.0
To: Python Mailing List <python-list@python.org>
Subject: Re: Critic my module
References: <mailman.5092.1374758683.3114.python-list@python.org> <51f334eb$0$29971$c3e8da3$5496439d@news.astraweb.com>
In-Reply-To: <51f334eb$0$29971$c3e8da3$5496439d@news.astraweb.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: list
Newsgroups: comp.lang.python
Message-ID: <mailman.5167.1374929783.3114.python-list@python.org>
Lines: 239
NNTP-Posting-Host: 2001:888:2000:d::a6
Xref: csiph.com comp.lang.python:51352


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.
>
>
Wow! Thanks for the thorough critic. I appreciate your feed back and 
thank you so much for the PEP link. I learned a lot. I never saw that 
page before.

The "NCLA, Linux+, LPIC-1, DCTS" are my computer certifications. As for 
mentioning Geany, I am trying to promote and give credit to Geany.

Good point about the Made by/Copyright suggestion. Although, I have not 
copyrighted the file, can I still say "Copyrighted by ...". Thank you 
for the LGPLv3 suggestion. I know that I must include the GPL license 
for GPL programs, but I thought for LGPL code I could just have 
"#LGPLv3". Thank you so much for that feedback. I definitely need to 
read about all of the types of licenses.

I thought it would be helpful to include the version numbers for each 
function, but you and another Python developer said it is pointless. I 
see what you mean.

The grep emulating function does not work yet. I am still working on that.

Yeah, I have a VERY BAD habit of treating bash and the Linux shell (or 
any/all shells) as the same thing. I know they are all very different, 
but for some reason I still keep calling all shells in general BASH.

I will be sure to create aliases. I make alias commands so that it is 
easier to guess or remember a command. For instance, a Python user my 
want to clear the shell's history, but can only remember one form of the 
command or must guess. On my Ubuntu system, I have set up numerous shell 
aliases. I am addicted to aliases.

I still need to add the other browsers. Do very many people use Iceweasel?

I did not notice that I have "print(subprocess.Popen('(xterm &)'))" 
instead of "subprocess.Popen('(xterm &)')". The worst computer errors 
are ID-10-T errors.

True, the user my have Xterm open, but what if they use Guake (like me) 
or Pterm, EvilVTE, Valaterm, Gnome-Terminal, Konsole, etc.?

How could I add security and convenience? Okay, I will try to add wget. 
Are there any other shell commands that anyone feels I should add?

The point of this module is to allow Linux shell users to use Python3 as 
a regular shell. Instead of using CSH, Bash, Tcsh, FISH, etc., users 
could use Python3 and import this module. Python is more powerful than 
any shell, so I want to make it easier for anyone to use Python as the 
default shell. For instance, instead of typing "print(os.getcwd())" to 
get the current working directory, users could type "boash.ls()". I hope 
that is easier to remember than "print(os.getcwd())". As for the print() 
command, I do not like how os.getcwd() has single quotes around the 
output. Plus, Linux shell do not print output with quotes.

I want to make this a very useful and popular module, so I will use the 
suggestions and add more useful wrappers. Would it help if I made a 
Youtube video showing how this module can be used?

I will post the next version on this mailing list for another review. 
Thanks everyone, and thanks a lot Steven D'Aprano!


Mahalo,

Devyn Collier Johnson
DevynCJohnson@Gmail.com