5
\$\begingroup\$

Original post here. I've taken @ThomasWard's advice, and also added a couple other things.

#!/usr/bin/env python

import tokenize
import sys
import json


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="assignments.txt", open_from="peoples.txt", \
                                        to_write=True, to_exec=True):

    '''
    - `assignments` is the file to open the list of names from
    - `open_from` is the file to get the input code from
    - `to_write` is for toggling writing the compiled code to a file
    - `to_exec` is for toggling executing the code

    Both `to_write` and `to_exec` are for using this code in another
    file by way of importing it.
    '''

    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)

    if to_write:
        with open(open_from[:-4]+"-output.txt", 'w') as outfile:
            # Write to the output file
            outfile.write(output)

    if to_exec:
        exec output in globals(), locals()

    return output


if __name__ == "__main__":
    token_names = None
    try:
        if len(sys.argv) > 1:
            if len(sys.argv) > 2:
                run(assignments=sys.argv[1], open_from=sys.argv[2])
            else:
                run(assignments=sys.argv[1])
        else:
            run()

    except Exception as e:
        print "An exception has occurred:\n%s" % str(e)

The contents of assignments.txt:

{"Martin":"False",
"Geobits":"None",
"Dennis":"True",
"adnan":"and",
"rainbolt":"as",
"buttner":"assert",
"flawr":"break",
"aditsu":"class",
"katenkyo":"continue",
"quill":"def",
"nathan":"del",
"hobbies":"elif",
"helkahomba":"else",
"irk":"except",
"ender":"finally",
"peter":"for",
"conor":"from",
"gnibbler":"global",
"calvins":"if",
"obrien":"import",
"taylor":"in",
"fryamtheeggman":"is",
"starman":"lambda",
"sp3000":"nonlocal",
"phinotpi":"not",
"xnor":"or",
"maltysen":"pass",
"mego":"raise",
"alex":"return",
"easterly":"try",
"molarmanful":"while",
"minxomat":"with",
"optimizer":"yield",
"wheat":"__import__",
"mbomb007":"abs",
"digital":"all",
"trauma":"any",
"asciionly":"ascii",
"zyabin":"bin",
"bkul":"bool",
"chris":"chr",
"jesteryoung":"classmethod",
"honnza":"complex",
"elendia":"enumerate",
"gcampbell":"eval",
"businesscat":"exec",
"poke":"dir",
"votetoclose":"delattr",
"fatalize":"filter",
"ataco":"float",
"luke":"frozenset",
"betseg":"hasattr",
"sandbox":"help",
"christopherpeart":"hex",
"zgarb":"id",
"phase":"input",
"loovjo":"int",
"minibits":"issubclass",
"lynn":"len",
"laikoni":"list",
"doorknob":"map",
"upgoat":"max",
"briantompsett":"memoryview",
"downgoat":"min",
"jimmy23013":"open",
"destructiblewatermelon":"ord",
"jan":"oct",
"ninjabearmonkey":"pow",
"you":"print",
"productions":"property",
"djmcmayhem":"range",
"erik":"repr",
"qwerpderp":"round",
"legionmammal978":"set",
"lliwtelracs":"slice",
"orlp":"sorted",
"timmyd":"staticmethod",
"eth":"str",
"muddyfish":"sum",
"balint":"super",
"trichoplax":"tuple",
"quartata":"zip",
"pavel":"sys"}

And for a test case (store in peoples.txt):

peter i taylor djmcmayhem(10):
    you(i)

Should result in:

for i in range(10):
    print(i)

Info and how to run:

What does the code do? It takes in an input file (by default peoples.txt in the same dir as main.py) and translates it from "People's Python" to standard python, and executes the result. It also writes the compiled "normal" code into <input_file_name>-output.txt, by default peoples-output.txt

Run as python main.py custom/assignments.txt path/to/inputfile.txt (inputfile.txt is the People's Python code and assignments.txt is the dict of assignments you're using)

To easily test this, put main.py (the first code block), assignments.txt (the big JSON/dict), and the test case (peoples.txt) all in the same directory, and run python main.py.


The main things I'm concerned about are the exec calls and the system arguments. In addition to general best practices, how can I clean up those particular parts?

\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$

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)
\$\endgroup\$
2
  • \$\begingroup\$ You should use sys.executable instead of hardcoding the name of the interpreter. There is also no need to use the Popen interface as check_output (or run starting from python 3.5) will provide a nicer interface: execout = subprocess.check_output([sys.executable, writepath]); print execout. No need to check for execerr as check_output will do for us in a better way; but you need to enclose that in a try: finally: to remove writepath whatever happens. \$\endgroup\$ Feb 25, 2017 at 14:21
  • \$\begingroup\$ @MathiasEttinger thanks for your suggestions! I don't like check_output, personally, which is why I used Popen. Since OP uses Python 2, run isn't usable here. The rest is otherwise accurate, thanks! Though, since I have no internet and am on my phone, I can't reliably edit my review just now. :/ \$\endgroup\$ Feb 25, 2017 at 17:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy