1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You do know that all POSIX compliant shells have the 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\$ – Graipher Jan 28 '18 at 19:40
  • 1
    \$\begingroup\$ I do. But I think you missed a bigger picture here. \$\endgroup\$ – Nordenheim Jan 28 '18 at 20:08
  • \$\begingroup\$ If I thought this could replace your whole code, I would have made it an answer. This was just to check if you knew the obvious alternatives for some of it. \$\endgroup\$ – Graipher Jan 29 '18 at 6:03
1
\$\begingroup\$

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

\$\endgroup\$

Your Answer

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

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