I've just started learning Python 3 about a month ago. I created a tool, which allows the user to quickly access some long commands.
For instance, at my day job I have to type things like these a couple times a day:
ssh -i /home/user/.ssh/id_rsa user@server
docker exec -i mysql_container_name mysql -u example -pexample example < example.sql
This really annoys me, so I created a tool which would allow me to run a ssh
or a import
and save me a lot of time.
But since I'm new to Python, I seek tips on how to improve my code.
import re
import os
import sys
import yaml
from a.Helper import Helper
class A(object):
_argument_dict = {}
_argument_list = []
_settings = {}
# Run a command
def run(self, command, arguments):
self._load_config_files()
self._validate_config_version()
self._separate_arguments(arguments)
self._initiate_run(command)
# Reparse and return settigns
def get_settings(self):
self._load_config_files()
self._validate_config_version()
return self._settings
# Load all the config files into one dictionary
def _load_config_files(self):
default_settings = {}
local_settings = {}
try:
default_settings = self._load_config_file(os.path.dirname(__file__))
except FileNotFoundError:
print("Can't locate native alias.yml file, the app is corrupt. Please reinstall.")
sys.exit()
cwd_list = os.getcwd().split('/')
while not cwd_list == ['']:
path = "/".join(cwd_list)
try:
local_settings = self._merge_settings(local_settings, self._load_config_file(path))
except FileNotFoundError:
pass
cwd_list = cwd_list[:-1]
self._settings = self._merge_settings(default_settings, local_settings)
# Load a specific config file from specific location
def _load_config_file(self, path):
with open(path + "/alias.yml", "r") as stream:
try:
config = yaml.load(stream)
config = self._reparse_config_with_constants(config, path)
return config
except yaml.YAMLError as ex:
print(ex)
sys.exit()
# Go over the configs and substitute the so-far only one constant
def _reparse_config_with_constants(self, config, path):
try:
for commands in config['commands']:
if isinstance(config['commands'][commands], str):
config['commands'][commands] = config['commands'][commands].replace("{{cwd}}", path)
elif isinstance(config['commands'][commands], list):
for id, command in enumerate(config['commands'][commands]):
config['commands'][commands][id] = command.replace("{{cwd}}", path)
except KeyError:
pass
return config
# Merge the settings so that all of them are available.
def _merge_settings(self, source, destination):
for key, value in source.items():
if isinstance(value, dict):
node = destination.setdefault(key, {})
self._merge_settings(value, node)
else:
destination[key] = value
return destination
# Parse arguments to dictionary and list. Dictionary is for named variables, list is for anonymous ones.
def _separate_arguments(self, arguments):
prepared_dict = []
for argument in arguments:
match = re.match(r"--([\w]+)=([\w\s]+)", argument)
if match:
prepared_dict.append((match.group(1), match.group(2)))
else:
self._argument_list.append(argument)
self._argument_dict = dict(prepared_dict)
# Die if yaml file version is not supported
def _validate_config_version(self):
if self._settings['version'] > self._settings['supported_version']:
print("alias.yml version is not supported")
sys.exit()
# Prepare and run specific command
def _initiate_run(self, command_name):
try:
command_list = self._settings['commands'][command_name]
argument_list_cycler = iter(self._argument_list)
# Replace a variable name with either a value from argument dictionary or from argument list
def replace_variable_match(match):
try:
return self._argument_dict[match.group(1)]
except KeyError:
return next(argument_list_cycler)
# Replace and
def run_command(command):
command = re.sub(r"%%([a-z_]+)%%", replace_variable_match, command)
os.system(command)
if isinstance(command_list, str):
run_command(command_list)
elif isinstance(command_list, list):
for command in command_list:
run_command(command)
else:
Helper(self._settings)
except StopIteration:
print("FATAL: You did not specify the variable value for your command.")
sys.exit()
except IndexError:
Helper(self._settings)
sys.exit()
except KeyError:
Helper(self._settings)
sys.exit()
Quick config explanation
The tool allows the user to create a alias.yml
file in some directory and all the commands the user specifies there will be available in any subdirectory. The config file (alias.yml
) might contain either one command as string or a list of commands and has to look like this:
version: 1.0 # For future version, which could introduce breaking changes
commands:
echo: echo Hello World
ssh:
- scp file user@remote:/home/user/file
- ssh user@remote
I introduced a possibility to use variables in %%variable%%
format in the specified commands and the user is required to specify them on run. For instance:
commands:
echo: echo %%echo%%
This will require the user to type a echo Hello
to produce the output of Hello
.
And there is also a specific constant {{cwd}}
(as "config working directory) to allow the user to run path-specific commands. For example, we use php-cs-fixer
inside a specific directory and running the command invoking php-cs-fixer
will fail in any subdirectory. Thus the config has to be written as this:
command:
cs: {{cwd}}/cs/php-cs-fixer --dry-run
Since this config is located in /home/user/php/project
, {{cwd}}
gets substituted with that path and then /home/user/php/project/cs/php-cs-fixer
is being executed. This allows me to run a cs
even from /home/user/php/project/code/src/Entity/etc/etc
- you got the point.
Entrypoint
Whenever a
is invoked with arguments, __main
runs run
from the above class. I wanted to be more OOP-like, so the arguments passed to run
are from sys.argv
: a.run(sys.argv[1], sys.argv[2::])
.
Conclusion
I really wonder how to improve the A
class posted above, both from the architectural and structural point of view. But if you'd like to give more tips on improving the code in overall, the repository is here.
1 Answer 1
In general I see some great code for someone so new to the language. There is quite a bit code here, and I have but time for a few style notes.
Python has lots of great ways to iterate
Here is a loop that really caught my eye. The iterating variable is being continually used to reaccess the iterated data, and thus ends up with a lot of boilerplate. This generally harms readability.
for commands in config['commands']:
if isinstance(config['commands'][commands], str):
config['commands'][commands] = config['commands'][commands].replace("{{cwd}}", path)
elif isinstance(config['commands'][commands], list):
for id, command in enumerate(config['commands'][commands]):
config['commands'][commands][id] = command.replace("{{cwd}}", path)
Continually reaccessing config['commands']
, when instead something like:
for name, command in config['commands'].items():
will allow access to the value at config['commands']
. This cleans up the code when that value is accessed:
for name, command in config['commands'].items():
if isinstance(command, str):
config['commands'][name] = command.replace("{{cwd}}", path)
elif isinstance(command, list):
command[:] = [c.replace("{{cwd}}", path) for c in command]
But in the case of mutable values, can clean up the assignment of those values as well:
command[:] = [c.replace("{{cwd}}", path) for c in command]
Note: I did not actually test this code, so it may hide some stupid typos.
No point in constructing an object that can not be used
In this construct (pared down):
default_settings = {}
try:
default_settings = self._load_config_file(os.path.dirname(__file__))
except FileNotFoundError:
sys.exit()
This line:
default_settings = {}
is unneeded, since it will always set in the case in which the program does not exit.
Intermediate Assignments:
Something like this:
node = destination.setdefault(key, {})
self._merge_settings(value, node)
I generally prefer something like:
self._merge_settings(value, destination.setdefault(key, {}))
If the name node
provides some important documentation function, or the expression is more complex, then an intermediate assignment can make some sense, but if not...
alias
command, which allows giving a name to a command? Also,ssh
has the~/.ssh/config
file where you can configure hosts (including ports and users). \$\endgroup\$