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}")
2 Answers 2
.pylect.password.py
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
).You should use globals to DRY hard coded values like
/Users/jfami/pylect
.You can use
pathlib
to have a modern path interface.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).
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
- You can use
a in {"1", "2"}
rather thana == "1" or a == "2"
. - Your list comprehension is too long and so is hard to read.
You can remove the comprehension entirely. - 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.
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)
Rather than using
"store_true"
and having to build theif
elif
else
we can use"store_const"
to storeMain.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>)
We can have a default main function by using
parser.set_defaults(main=no_entry)
.
We can then change-v
to store tomain
, rather thanversion
, 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>)
To add
list
we have two options:- As a mutually exclusive option with
--version
. - 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
- As a mutually exclusive option with
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'])
We need to pass the
Namespace
torun
(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.
-
\$\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\$192927376337929292283737373773– 1929273763379292922837373737732023年10月12日 18:05:07 +00:00Commented 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 useMain
rather thanMain()
everywhere, making tons of instances of the class is pointless. I have some concerns with.
and Python playing well together. \$\endgroup\$2023年10月12日 18:20:33 +00:00Commented 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\$192927376337929292283737373773– 1929273763379292922837373737732023年10月12日 18:21:57 +00:00Commented 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 sureimport
works correctly with.
in the name of files? (rhetorical, to explain my concern) \$\endgroup\$2023年10月12日 18:23:45 +00:00Commented 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\$192927376337929292283737373773– 1929273763379292922837373737732023年10月12日 18:29:24 +00:00Commented Oct 12, 2023 at 18:29
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.
-
\$\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\$192927376337929292283737373773– 1929273763379292922837373737732023年10月12日 18:07:44 +00:00Commented Oct 12, 2023 at 18:07
-
\$\begingroup\$ The
try
is there in case we don't have a password. \$\endgroup\$192927376337929292283737373773– 1929273763379292922837373737732024年10月07日 19:12:28 +00:00Commented Oct 7, 2024 at 19:12
Explore related questions
See similar questions with these tags.
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\$