4
\$\begingroup\$

This is the code for Pylect, a Python terminal I worked on a while ago which also caused my Desktop to get destroyed because of bad code. I use it infrequently but I would like a review for the code since this is a bit larger project that has span multiple months so there is some unclear bad code hidden in there likely.

Pylect is a REPL in which you provide Python code and Pylect executes the code and so on.

You can pass flags which are:
--name | Changes the name of the terminal.
--modules | Imports the comma-separated list of modules.
--dev | Shows debugging messages.

There are custom commands that Pylect has beginning with .:
.imports | Shows the imported modules.
.history | Shows the history so far.
.history prev | Shows the last item in the history.
.exit-env | Exits Pylect.
.clear-env | Clears all files in the Pylect folder except the Python files needed for Pylect.

There are also other things for fun (import ., import cake, import thanks, raise PylectError). 0.1+0.2 prints 0.3 is there because Python itself does the calculation wrong and there's also a reminder when I forgot the . before exit-env.

There is also a security feature where it has you type a password to access Pylect if there is a password. You can create a password, edit the password, and even remove the password through another program.

There is a configuration program for switching Pylect versions easily.

All text files involved:
.pylect.password.txt - Has the password.
.pylect.aliasversion.txt - Has the version which will be aliased to assuming that the .zprofile actually uses it for aliasing of course.

~/pylect/pylect1.1/.pylect.py

try:
    import getpass
    pw = open("/Users/jfami/pylect/.pylect.password.txt", "r").read()
    gpw = getpass.getpass(prompt="Password: ")
    if str(gpw) == str(pw):
        invalid = False
        pass
    else:
        print("Invalid password")
        invalid = True
except:
    invalid = False
    pass

if invalid == True:
    quit()

import sys, os, platform
from os import system as shell

modulenames = set(sys.modules) & set(globals())
allmodules = [sys.modules[name] for name in modulenames]

if "--dev" in sys.argv:
    dev = True
else:
    dev = False
if "--name" in sys.argv:
    try:
        if dev: print(f"[Pylect] name = {sys.argv[sys.argv.index('--name')+1]}")
        name = sys.argv[sys.argv.index("--name")+1]
    except:
        if dev: 
            print("[Pylect] name could not be initalized, choosing default name")
            print('[Pylect] name = "Pylect"')
        name = "Pylect"
else:
    if dev: print(f'[Pylect] name = "Pylect"')
    name = "Pylect"
if "--modules" in sys.argv:
    try:
        modules = sys.argv[sys.argv.index("--modules")+1]
        modules = modules.split(",")
    except:
        if dev:
            print("[Pylect] modules could not be initalized, ignoring option")
        modules = []
else:
    modules = []

class Pylect():
    def __init__(self):
        self.name = "Pylect"
        self.env_folder = "pylect1.1"
        self.version_tuple = (1, 1, 6)
        self.version_status = "release"
        self.version = "1.1.6"
        self.build = 49
        self.src = "python"
        self.src_path = "/Users/jfami/pylect/pylect1.1"
        self.modules = allmodules
        self.__pylect__ = self

del __file__
sys.pylect = Pylect()
sys.exec_context = (os.getcwd(), (platform.system(), platform.release()), (platform.uname().machine, platform.uname().node), (platform.python_implementation(), platform.python_compiler(), platform.python_build()), (sys.pylect.name, sys.pylect.env, sys.pylect.version))
sys.env = ("3.11", "pylect1.1")
sys.filename = ""
sys.validargs = ["--dev", "--name", "--modules"]
sys.pylectargs = []
sys.invalidargs = False
sys.ispylect = True
sys.argv.remove(sys.argv[0])
for arg in sys.argv:
    if arg.startswith("--"):
        sys.pylectargs.append(arg)
        if not arg == "--dev":
            try:
                if sys.argv[sys.argv.index(arg)+1].startswith("--"): raise Exception
                sys.pylectargs.append(sys.argv[sys.argv.index(arg)+1])
                sys.argv.remove(sys.argv[sys.argv.index(arg)+1])
            except:
                sys.invalidargs = True
        sys.argv.remove(arg)
if dev and sys.invalidargs: print("Some args could not be initalized")
print(f"{name} Terminal!")

def exit_env():
    from os import walk, remove
    from os.path import join
    path = '/Users/jfami/pylect/pylect1.1'
    items = [join(root, file) for root, subdirs, files in walk(path) for file in files]
    if len(items) == 1: return
    for item in items:
        if "pylect.py" in item:
            pass
        else:
            remove(item)

cmds = []

while True:
    if modules != []:
        for module in modules:
            if dev: print(f"[Pylect] import {module}")
            try:
                exec(f"import {module}", globals(), locals())
            except:
                print(f"No module named '{module}'")
    inp = input("> ")
    if dev and not inp[0] == ".": print(f"[Pylect] {inp}")
    cmds.append(inp)
    if inp == ".imports":
        modulenames = set(sys.modules) & set(globals())
        allmodules = [sys.modules[name] for name in modulenames]
        print(allmodules)
        continue
    elif inp == ".env":
        import sys
        print(f"Python {sys.version.split(' ')[0]} (pylect1.1)")
        continue
    elif inp == ".exit-env":
        if dev: print("[Pylect] exit_env()")
        if dev: print("[Pylect] exit()")
        exit_env()
        exit()
    elif inp == ".clear-env":
        if dev: print("[Pylect] exit_env()")
        exit_env()
        continue
    elif inp == ".history":
        if dev: print("[Pylect] cmds")
        print(cmds)
        continue
    elif inp == ".history prev":
        if dev: print("[Pylect] cmds[-2]")
        print(cmds[-2])
        continue
    elif inp == "import .":
        print("This is not a module :)")
        continue
    elif inp == "import cake":
        print("THE CAKE IS A LIE!")
        continue
    elif inp == "import thanks":
        print("Thank you")
        continue
    elif inp == "0.1+0.2":
        print(0.3)
        continue
    elif inp == "raise PylectError":
        print("PylectError: Could not find `PylectError`")
        continue
    elif inp == "exit-env":
        print("You forgot a dot.")
        continue
    try:
        res = eval(inp, globals(), locals())
        print(res)
    except KeyboardInterrupt:
        exit_env()
        exit()
    except:
        try:
            exec(inp, globals(), locals())
        except Exception as e:
            print(str(e))

~/pylect/pylect1.1/.pylect.password.py

import os

if os.path.exists("/Users/jfami/pylect/.pylect.password.txt"):
    change = input("Do you want to change your password? (y/n) ")
    if change == "y":
        with open("/Users/jfami/pylect/.pylect.password.txt", "w") as pw:
            password = input("Input your new password: ")
            pw.write(password)
    else:
        remove = input("Do you want to remove your password? (NOT RECOMMENDED) (y/n) ")
        if remove == "y":
            os.remove("/Users/jfami/pylect/.pylect.password.txt")
else:
    setup = input("Setup a Pylect password? (y/n) ")
    if setup == "y":
        with open("/Users/jfami/pylect/.pylect.password.txt", "w") as pw:
            password = input("Input your password: ")
            pw.write(password)

~/pylect/pylect1.1/.pylect.config.py

import os
import subprocess
import sys

if sys.argv[1] == "list" or sys.argv[1] == "-l":
    print("Pylect: Finding avaliable versions...")
    versions = [x.split("pylect")[1] for x in subprocess.check_output("ls ~/pylect", shell=True).decode().strip().split("\n")]

    for version in versions:
        if open("~/pylect/.pylect.aliasversion.txt", "r").read().strip() == version:
            print(version, "(current)")
        else:
            print(version)
elif sys.argv[1] == "version" or sys.argv[1] == "-v":
    print(f"Pylect {open("~/pylect/.pylect.aliasversion.txt", "r").read().strip()} (pylect{open("~/pylect/.pylect.aliasversion.txt", "r").read().strip()})")
elif sys.argv[1] == "run" or sys.argv[1] == "-r":
    args = ""
    for arg in sys.argv[1:]:
        args += f" {arg}"
    os.system(f"pylect {args}")
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I think the quality of your question would improve if you added more as a description. At various points during answering I thought "why are you even doing X". Some parts elif inp == "0.1+0.2": seem to be nothing more than a meme so make understanding what is and isn't important hard. \$\endgroup\$
    – Peilonrayz
    Oct 12, 2023 at 18:11
  • \$\begingroup\$ I will add that, to answer your question they are for fun except the 0.1+0.2 part because Python often incorrectly calculates that and the "You forgot a dot" one is to remind me that I forgot a dot when exiting. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:18
  • \$\begingroup\$ Also since I know what the code does and why it's hard for me to know which parts you were confused at, it would be helpful if you would provide which parts you were confused at. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:20
  • 2
    \$\begingroup\$ How to get the best value out of Code Review - Asking Questions \$\endgroup\$
    – Mast
    Oct 12, 2023 at 19:24
  • 2
    \$\begingroup\$ I don't see anything here that emulates a terminal. Rather, it looks like a shell program that assumes standard streams are already connected to a terminal (or terminal emulator). Have you misunderstood the terminology? \$\endgroup\$ Oct 13, 2023 at 7:02

2 Answers 2

5
\$\begingroup\$

.pylect.password.py

  1. Your file naming is non-standard. I have some concerns with prefixing python files with ., and using . rather than _ to delimit names.

    Make pylect1.1 hidden instead (.pylect1.1).

  2. You should use globals to DRY hard coded values like /Users/jfami/pylect.

  3. You can use pathlib to have a modern path interface.

  4. You should use functions. You have two places which write to a password file. You can make a function to reduce the amount of repeated code (DRY).

  5. You can DRY your code by adding a SET_TEXT constant with both versions of the set text.

import pathlib

ROOT = pathlib.Path("/Users/jfami/pylect")
PASSWORD = ROOT / ".pylect.password.txt"
SET_TEXT = (
    (
        "Setup a Pylect password? (y/n) ",
        "Input your password: ",
    ),
    (
        "Do you want to change your password? (y/n) ",
        "Input your new password: ",
    ),
)


def set_password(
    password: str,
    location: str | pathlib.Path = PASSWORD,
) -> None:
    with open(location, "w") as f:
        f.write(password)


def main() -> None:
    exits = PASSWORD.exists()
    SET, PASSWORD = SET_TEXT[exists]
    if "y" == input(SET):
        set_password(input(PASSWORD))
    elif exists:
        remove = input("Do you want to remove your password? (NOT RECOMMENDED) (y/n) ")
        if remove == "y":
            os.remove(PASSWORD)

.pylect.config.py

We can take a lot of the learning points from .pylect.password.py

  1. You can use a in {"1", "2"} rather than a == "1" or a == "2".
  2. Your list comprehension is too long and so is hard to read.
    You can remove the comprehension entirely.
  3. We can use " ".join(sys.argv[1:]) rather than a for loop to in run.
import os
import pathlib
import subprocess
import sys

ROOT = pathlib.Path("~/pylect")
ALIAS_VERSION = ROOT / ".pylect.aliasversion.txt"


def read_version(path: str | pathlib.Path = ALIAS_VERSION):
    with open(ALIAS_VERSION, "r") as f:
        return f.read().strip()


class Main:
    @staticmethod
    def list() -> None:
        print("Pylect: Finding avaliable versions...")
        for line in subprocess.check_output("ls ~/pylect", shell=True).decode().strip().split("\n"):
            version = line.split("pylect")[1]
            if version == read_version():
                print(version, "(current)")
            else:
                print(version)

    @staticmethod
    def version() -> None:
        version = read_version()
        print(f"Pylect {version} (pylect{version})")

    @staticmethod
    def run() -> None:
        os.system(f"pylect {' '.join(sys.argv[1:])}")


if sys.argv[1] in {"list", "-l"}:
    Main.list()
elif sys.argv[1] in {"version", "-v"}:
    Main.version()
elif sys.argv[1] in {"run", "-r"}:
    Main.run()

I think you could possibly benefit from argparse to make more complicated CLI interfaces easier.

  1. We can add a flag to allow using -v and --version as to get the version information:

    parser = argparse.ArgumentParser()
    parser.add_argument("-v", "--version", action="store_true")
    print(parser.parse_args(["-v"]))
    
    Namespace(version=True)
    
  2. Rather than using "store_true" and having to build the if elif else we can use "store_const" to store Main.version.

    parser = argparse.ArgumentParser()
    parser.add_argument("-v", "--version", action="store_const", const=Main.version)
    print(parser.parse_args(["-v"]))
    
    Namespace(version=<function Main.version at 0x7f0c36e99440>)
    
  3. We can have a default main function by using parser.set_defaults(main=no_entry).
    We can then change -v to store to main, rather than version, to overwrite the function we run.

    parser = argparse.ArgumentParser()
    parser.set_defaults(main=Main.no_entry)
    parser.add_argument("-v", "--version", action="store_const", dest="main", const=Main.version)
    print(parser.parse_args([]))
    print(parser.parse_args(["-v"]))
    
    Namespace(main=<function Main.no_entry at 0x7f7bfde10f40>)
    Namespace(main=<function Main.version at 0x7f7bfdf9d440>)
    
  4. To add list we have two options:

    1. As a mutually exclusive option with --version.
    2. Add a subparser.

    Since run will be a subparser I'll make --list mutually exclusive with --version as a means of demonstration.

    parser = argparse.ArgumentParser()
    parser.set_defaults(main=Main.no_entry)
    
    options = parser.add_mutually_exclusive_group()
    options.add_argument("-v", "--version", action="store_const", dest="main", const=Main.version)
    options.add_argument("-l", "--list",    action="store_const", dest="main", const=Main.list)
    
    print(parser.parse_args(["-v"]))
    print(parser.parse_args(["-l"]))
    print(parser.parse_args(["-l", "-v"]))
    
    Namespace(main=<function Main.version at 0x7f4690c99440>)
    Namespace(main=<function Main.list at 0x7f4690c98f40>)
    usage: .pylect.py [-h] [-v | -l]
    .pylect.py: error: argument -v/--version: not allowed with argument -l/--list
    
  5. We can add run as a subparser.

    Subparsers are very similar to the root parser so the code should be fairly easy to understand.

    parser = argparse.ArgumentParser()
    parser.set_defaults(main=Main.no_entry)
    # ...
    subparsers = parser.add_subparsers()
    
    run = subparsers.add_parser("run")
    run.set_defaults(main=Main.run)
    run.add_argument("values", nargs="*")
    
    print(parser.parse_args(["run", "1", "2", "3"]))
    
    Namespace(main=<function Main.run at 0x7f26d91d1f80>, values=['1', '2', '3'])
    
  6. We need to pass the Namespace to run (and all other functions) to be able to get the same functionality.

        @staticmethod
        def run(args: argparse.Namespace) -> None:
            print(f"pylect {' '.join(args.values)}")
    
    
    args = parser.parse_args()
    args.main(args)
    

Note: functions have been changed for easier testing of argparse.

import argparse


class Main:
    @staticmethod
    def list(args: argparse.Namespace) -> None:
        print("Pylect: list")

    @staticmethod
    def version(args: argparse.Namespace) -> None:
        print(f"Pylect: version")

    @staticmethod
    def run(args: argparse.Namespace) -> None:
        print(f"pylect {' '.join(args.values)}")

    @staticmethod
    def no_entry(args: argparse.Namespace) -> None:
        print(f"pylect: no command specified")
        raise SystemExit(1)

    @classmethod
    def build_parser(cls) -> argparse.ArgumentParser:
        parser = argparse.ArgumentParser()
        parser.set_defaults(main=cls.no_entry)
        parser.add_argument(
            "-v",
            "--version",
            action="store_const",
            dest="main",
            const=cls.version,
        )
        subparsers = parser.add_subparsers()

        list = subparsers.add_parser("list")
        list.set_defaults(main=cls.list)

        run = subparsers.add_parser("run")
        run.set_defaults(main=cls.run)
        run.add_argument("values", nargs="*")
        return parser


args = Main.build_parser().parse_args()
args.main(args)

Some more can be said of .pylect.py. However I think you should expand on my advice and update the file for a 2nd review.

\$\endgroup\$
8
  • \$\begingroup\$ This code was written a while ago so I was dumber then and now I do stuff like make the folder hidden and not the files inside and I also use libraries to make argument parsing easier. Anyways this is a good answer and goes into more detail then the other answer. I feel like argparse is overkill for this and any reason you use static methods? Except that this sounds pretty good. Using . to delimit fits better then _ as it looks less ugly and breaks up it into parts of the file to say. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:05
  • \$\begingroup\$ @The_AH argparse is 100% overkill for .pylect.config.py. However I was the simpler situation as a tutorial if you need to learn argparse. As you then can add argparse to .pylect.py. Static methods are used because then we can use Main rather than Main() everywhere, making tons of instances of the class is pointless. I have some concerns with . and Python playing well together. \$\endgroup\$
    – Peilonrayz
    Oct 12, 2023 at 18:20
  • \$\begingroup\$ Oh that makes sense. If you don't have an init function you can just directly access a function instead of making an instance so I don't get the point of static methods then? What are you concerns with . because I clearly don't see them. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:21
  • \$\begingroup\$ @The_AH You seem to understand the point of static methods now. If you can't see my concern with . then don't worry for now. However are you sure import works correctly with . in the name of files? (rhetorical, to explain my concern) \$\endgroup\$
    – Peilonrayz
    Oct 12, 2023 at 18:23
  • \$\begingroup\$ No I don't see the point of static methods, it seems like a pointless decorator because it does what by default Python does. If I make a Main class with a hello function then Main.hello() will work along with Main().hello() so static methods are pointless. I know . doesn't work in imports but it's not like I am going to import those files and for files I need to import I will just avoid filenames like that. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:29
3
\$\begingroup\$

needlessly hardcoded pathname

    pw = open("/Users/jfami/pylect/.pylect.password.txt", "r").read()

personal project that I don't plan on releasing online so please avoid advice about hardcoded paths.

Earlier today you chose to release it online.

But imagine I was reading a printout of this in your living room rather than on a public StackExchange site. I would still advise you to code Bourne scripts with ${HOME}/pylect... and python scripts with the corresponding expression.

It's just a good habit, and it is helpful to cultivate such habits. Code as if this script won't be discarded tomorrow, or as if it won't be for your eyes only.

Besides, your current hosts, or one you acquire next month, might put home directories in /usr/home, /u, /Users, or diverse other places.

Express the hardcode as Path("~/pylect...").expanduser().


credentials stored in the clear

    if str(gpw) == str(pw):

It's hard to see why we do this "security" test at all, given that a user who can perform the equality test is a user who already has read permissions on the text file. Prefer to store SHA3(pw) or some similar hash.

The try is odd, unless we feel that import getpass may be unavailable on host(s) we execute this on, or perhaps that ctrl-D EOFError is of interest. In any event the bare except: is bad and should be replaced with except Exception: so ctrl-C isn't masked. Similarly in the "--modules" check. There seems to be little need for assigning to an invalid variable since we could do the quit() side effect instead.

Put the getpass import where it belongs, at top level outside the try.

Delete the useless pass -- we only write that in places where it is syntactically necessary.


isort

import sys, os, platform

Pep-8 essentially asks that you run "$ isort *.py", so these will be three lines in the proper order in the proper place within your module.


main guard

You have a lot of code at top level. Some of it produces side effects.

Bury it within def main(): or similar. And tack on the customary

if __name__ == "__main__":
    main()

That way other modules can import this one.

For example, you might break out a helper function and will then want a test suite to be able to execute it.


assign boolean expression

if "--dev" in sys.argv:
    dev = True
else:
    dev = False

Prefer:

dev = "--dev" in sys.argv

Better, use typer, or at least argparse. That way users get standard behavior, and --help, for free.


use standard syntax

        if dev: print(f"[Pylect] name = {sys.argv[sys.argv.index('--name')+1]}")

No.

Prefer:

        if dev:
            print(f"[Pylect] name = {sys.argv[sys.argv.index('--name')+1]}")

Also, now we're starting to see how importing typer or argparse would be a big win, simplifying a needlessly complex expression.


limited line length

I didn't read the end of your sys.exec_context = ... assignment, as due to a horizontal scrollbar it disappeared way off the right hand side of the window. You essentially told the reader, "don't read this". Use "$ black -S *.py" to repair this code's legibility.


explain what code does

You clean up and then assign many attributes in the sys module. This top level code lacks a # comment, and it is unclear what Author's Intent is. Therefore it is unclear whether this code is correct.

Write a comment, or better, define a helper function for it, and give that function a """docstring""".

Ok, that's as much as I can bear to read.

\$\endgroup\$
1
  • \$\begingroup\$ You go into some details that the other answer doesn't while remaining short! I don't use comments and I will write a docstring probably. To answer the changing of the sys module, I clean up some stuff and add some stuff that I often access long with information about Pylect of course and it is correct. You should assume all of the code here is correct as I had read it before posting and tried it out. You have good advice with not hardcoded paths there and I will add that. \$\endgroup\$
    – The_AH
    Oct 12, 2023 at 18:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.