trjhtr

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]+)=([ws]+)", 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.

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...