Welcome to Review, Round #2. I'm gonna be more harsh this time around heh. At least, with regards to style. Except for PEP8 line lengths - I'm using my 120-max-chars-per-line preference (which is acceptable under PEP8 if I'm the only one working on the code, or the 'team' is OK with it)
Unnecessary backslashes for line continuations between parentheses
def run(assignments="assignments.txt", open_from="peoples.txt", \ to_write=True, to_exec=True):
This is what you have, above. You don't need that backslash when everything is contained between parentheses. You also need to make it line up correctly for indentation from a style perspective. That should be this:
def run(assignments="assignments.txt", open_from="peoples.txt", to_write=True, to_exec=True):
Some Docstring Issues
No need for a space between def and docstring
This is more or less some semantics and annoyances, but you don't need an extra space here. It's quite proper to have the docstring right up against the def line.
Use """ instead of '''!
Docstrings should use triple-quotes, not triple-apostrophes.
Use proper Docstring Format
Your docstrings don't conform to typical formats. Use this instead for the run docstring:
""" Execute the conversion from People's Python to Standard Python :param assignments: String denoting the path to open the name list from. :param open_from: String denoting the file to get People's Python code input from. :param to_write: True/False to determine whether we store the converted code to a file. :param to_exec: True/False to determine whether we execute the converted code. :return: The converted code. """
NOTE: This is one of multiple formats, but this is able to be parsed by a documentation / helpdoc generator without much issue, at least via my IDE. A lot of my recommendations end up from my IDE :P
Consider using argparse instead of sys.argv
I strongly recommend the use of argparse instead of relying on sys.argv. This way, you can have a better definition of runtime arguments and default values. It changes your execution a little, but is a little better, in my opinion, because you can capture 'unknown arguments' as errors, and provide a 'usage' description.
(1) import argparse at the beginning.
(2) I suggest making a 'get_args()' function, which returns the arguments so we can store it later. We can also override your 'execute' or 'write' actions later by defining --no-ACTION-output where 'ACTION' is either write or exec:
def get_args(): argparser = argparse.ArgumentParser(description="People's Python - Convert 'people' python to actual Python", add_help=True) argparser.add_argument('--assignments', required=False, type=str, dest='assignments', default="assignments.txt", help="Assignments file for names to Python builtins. Default is " "'assignments.txt'") argparser.add_argument('--people', '--peoples', required=False, type=str, dest='peoples', default="peoples.txt", help="People's Python file, for conversion. Default is 'peoples.txt'.") argparser.add_argument('--no-write-output', required=False, dest='write', action="store_false", default=True, help="Do not write output from conversion to standard python to a file, " "if this argument is provided.") argparser.add_argument('--no-exec-output', required=False, dest='execute', action="store_false", default=True, help="Do not execute output from conversion and do whatever it is set to do, " "if this argument is provided.") return argparser.parse_args()
(3) Call this function to parse the arguments, and then just pass a single 'run' call instead of testing number of args.
if __name__ == "__main__": token_names = None args = get_args() try: run(args.assignment, args.peoples, args.write, args.execute) except Exception as e: print "An exception has occurred:\n%s" % str(e)
By doing this, also, we don't need to define 'defaults' for 'optional arguments' for run either, because we already specify default values when we parse arguments. So, the definition for run becomes this instead:
def run(assignments, open_from, to_write, to_exec):
Store the 'writepath' instead of just doing all the changes in the 'open' field.
This is necessary for one of my later suggestions, but we can save the path for execution for reuse later on.
writepath = open_from[:-4]+"-output.txt" if to_write: with open(writepath, 'w') as outfile: # Write to the output file outfile.write(output) outfile.close() if to_exec: # ...
Consider doing an execution in a subprocess instead of exec directly; if to_write is false, use a temporary file for this
Mostly because I dislike calling exec directly on string data, and would rather use data from a file, either a temporary one or the one we wrote already, this suggestion is written.
While I tend to be a little evil here, this is more or less personal preference (it also works on my Linux environment, sorry it's not a cross-platform recommendation currently). I'd rather spawn a secondary process with a temporary file (if to_write is False) to handle execution. That way, you can not break your existing process and get other useful data. This sometimes adds an extra line break in the output if there is any, but it works pretty decently for your test case.
This also requires the storing of 'writepath' with the use of /tmp/ on Linux environments and Mac environments... and importing subprocess as sp
# At the beginning with your imports: import subprocess as sp # Later on in the code... if to_exec: if not to_write: writepath = '/tmp/' + writepath with open(writepath, 'w') as outfile: # Write to the output file outfile.write(output) outfile.close() (execout, execerr) = sp.Popen(str('python %s' % writepath).split(), stdout=sp.PIPE, stdin=sp.PIPE).communicate() if not to_write: os.remove(writepath) if execerr and execerr != '': raise RuntimeError("An issue occurred running the converted code:\n%s" % str(execerr)) print execout
This is the final code with these recommendations:
#!/usr/bin/env python import tokenize import json import os import argparse import subprocess as sp def get_args(): argparser = argparse.ArgumentParser(description="People's Python - Convert 'people' python to actual Python", add_help=True) argparser.add_argument('--assignments', required=False, type=str, dest='assignments', default="assignments.txt", help="Assignments file for names to Python builtins. Default is " "'assignments.txt'") argparser.add_argument('--people', '--peoples', required=False, type=str, dest='peoples', default="peoples.txt", help="People's Python file, for conversion. Default is 'peoples.txt'.") argparser.add_argument('--no-write-output', required=False, dest='write', action="store_false", default=True, help="Do not write output from conversion to standard python to a file, " "if this argument is provided.") argparser.add_argument('--no-exec-output', required=False, dest='execute', action="store_false", default=True, help="Do not execute output from conversion and do whatever it is set to do, " "if this argument is provided.") return argparser.parse_args() def handle_token(type_, token, (srow, scol), (erow, ecol), line): # Return the info about the tokens, if it's a NAME token then replace it if tokenize.tok_name[type_] == "NAME": token = token_names.get(token, token) return type_, token, (srow, scol), (erow, ecol), line def run(assignments, open_from, to_write, to_exec): """ Do the conversion from People's Python to Standard Python :param assignments: String denoting the path to open the name list from. :param open_from: String denoting the file to get People's Python code input from. :param to_write: True/False to determine whether we store the converted code to a file. :param to_exec: True/False to determine whether we execute the converted code. :return: The converted code. """ with open(assignments, "r") as f: # Read the replacements into token_names global token_names token_names = json.load(f) with open(open_from) as source: # Get the tokenized version of the input, replace it, and untokenize into pretty output tokens = tokenize.generate_tokens(source.readline) handled_tokens = (handle_token(*token) for token in tokens) output = tokenize.untokenize(handled_tokens) writepath = open_from[:-4]+"-output.txt" if to_write: with open(writepath, 'w') as outfile: # Write to the output file outfile.write(output) if to_exec: if not to_write: writepath = 'tmp.' + writepath with open(writepath, 'w') as outfile: # Write to the output file outfile.write(output) outfile.close() (execout, execerr) = sp.Popen(str('python %s' % writepath).split(), stdout=sp.PIPE, stdin=sp.PIPE).communicate() if not to_write: os.remove(writepath) if execerr and execerr != '': raise RuntimeError("An issue occurred running the converted code:\n%s" % str(execerr)) print execout return output if __name__ == "__main__": token_names = None args = get_args() try: run(args.assignments, args.peoples, args.write, args.execute) except Exception as e: print "An exception has occurred:\n%s" % str(e)
