4
\$\begingroup\$

I writing tool to manage our application.

One part of it - is INI-file, which contains UUIDs for some application modules.

It looks like:

[version_1_0_2_staging]
MATH_DLL = 7a00ca94-b68d-4c89-a004-d3a4f5e7cf56
MATH_DLL_XML = 738174f7-f310-4989-8cae-da1690487e7c
ASSETMANAGER_API_DLL = 2e9e3807-4f8a-4ae1-a873-4a8f6d152eaf
ASSETMANAGER_API_DLL_XML = 024130cf-cbd9-41c2-b432-db9bc8565220
...
[debug]
MATH_DLL = 7b73c8f2-c5b6-435d-b310-37628302d1a0
MATH_DLL_XML = 5a349f50-76c3-456e-a885-f893c501a050
NETWORK_IMPL_DLL = 90a7b774-aa9a-4b85-b5e7-822333860588
...

And so on.

Script have few parts. Main is RDSmanager.py, where options defined:

parser_unity = subparsers.add_parser('unity', help='Unity application')
parser_unity.set_defaults(func=handler_unity)
...
    # UUIDs management
parser_unity.add_argument('-u', '--uuidgen', action='store', dest='unity_uuidgen',
                          help='Generate UUIDs for each module in Plugins.sln. Must be called with \'config_name\' as first argument and optionally - with `-b ci` option')
parser_unity.add_argument('-l', '--configlist', action='store_true', dest='unity_configlist',
                          help='Display configuration names in uuids.ini')
parser_unity.add_argument('-x', '--remove', action='store', dest='unity_uuid_remove',
                          help='Remove configuration from uuids.ini. Must be called with config_name to delete')
...

Than - function to parse options:

def handler_unity(action_list):
...
   if action_list.unity_uuidgen:
        logger.logger.info('Running UUID generator with %s' % action_list.unity_uuidgen)
        from lib.unity.uuidgen import UuuidManagment
        u = UuuidManagment(logger, RDS_BASEDIR)
        u.uuidgen(action_list.unity_uuidgen, action_list.unity_buildtype)

    if action_list.unity_configlist:
        logger.logger.info('Running configlist')
        from lib.unity.uuidgen import UuuidManagment
        u = UuuidManagment(logger, RDS_BASEDIR)
        u.configlist()

    if action_list.unity_uuid_remove:
        from lib.unity.uuidgen import UuuidManagment
        u = UuuidManagment(logger, RDS_BASEDIR)
        u.config_remove(action_list.unity_uuid_remove)

And finally - this very class:

#!/usr/bin/env python

import uuid
import os
import sys
import subprocess

from lib.external import ConfigParser
from lib.unity.external import modules_list, checkhome, mod_proj
from lib.external.external import isdev
from lib.unity.tag_checker import parser
from lib.external.exceptions import *

"""Generates UUIDs for modules and module's XML files.
    Will save output to file RDSmanager/conf/unity/uuids.ini
    Must be enclosed with configuration name, e.g. 'beta_1_1', 'debug' etc."""


class UuuidManagment(object):

    """Class for data in uuids.ini file management"""

    def __init__(self, logger, rds_basedir):

        self.config = ConfigParser.ConfigParser()
        self.config_file = os.path.join(rds_basedir, 'conf', 'unity', 'uuids.ini')
        self.config.read(self.config_file)
        self.configs = self.config.sections()

        self.logger = logger
        self.rds_basedir = rds_basedir

    def gitsave(self, outfile, config_name):

        """Save updated uuids.ini to repository"""

        os.chdir(self.rds_basedir)

        try:
            self.logger.logger.info('Updating uuids.ini list...')
            subprocess.call('git pull')
            self.logger.logger.info('Done.')
            subprocess.call('git add %s' % outfile)
            subprocess.call('git commit -m "Add new configuration set `%s`"' % config_name)
            subprocess.call('git push origin')
        except WindowsError as error:
            self.logger.logger.info('ERROR: %s' % error)
            sys.exit(1)

    def configlist(self):

        """Return list of actually created configs in uuids.list file"""

        if self.configs:
            self.logger.logger.info('\nCurrently avalable configurations: \n')
            for conf in self.configs:
                self.logger.logger.info(conf)
        else:
            raise NoConfigError(self.logger, 'No configs in %s found' % self.config_file)


    def uuidgen(self, config_name, build_type):

        """For new UUIDs set generation.
           Must be called with new config_name name"""

        workdir = checkhome(self.logger)
        modules = modules_list(workdir)

        if not os.path.isfile(self.config_file):
            raise NoConfigError(self.logger, 'No config file %s found' % self.config_file)

        if config_name in self.config.sections():
            self.logger.logger.info('Config with name %s already exist!\nYou can check all configs with RDSmanager -l option.' % config_name)
            sys.exit(1)

        self.config.add_section(config_name)

        for modname in modules:

            mod_proj_file = mod_proj(workdir, modname)

            try:
                mod_tag = parser(self.logger, modname, mod_proj_file)
                if mod_tag:
                    if 'CLOUD' in mod_tag:
                        if not isdev(build_type):
                            self.config.set(config_name, modname.upper() + '_DLL', str(uuid.uuid4()))
                            self.config.set(config_name, modname.upper() + '_DLL_XML', str(uuid.uuid4()))
                        else:
                            self.config.set(config_name, modname.upper() + '_DLL', modname + '.dll')
                            self.config.set(config_name, modname.upper() + '_DLL_XML', modname + '.dll' + '.xml')

            except AttributeError:
                pass

        additional = {
            'PRODUCTGROUPEDITOR_BUNDLE': 'ProductGroupEditor.unity3d',
            'CATALOGUE_BUNDLE': 'CatalogueBundle.unity3d',
            'EDITINGMODE_BUNDLE': 'EditingMenuBundle.unity3d',
            'SHELFEDITOR_BUNDLE': 'ShelfEditorBundle.unity3d',
            'MAIN_APPLICATION': 'MainApplication/web/web.unity3d',
            'REPLAY_BUNDLE': 'Replay.unity3d',
            'UNITYUI_BUNDLE': 'UnityUI.unity3d',
            'CONFIG_XML': 'config.xml'
        }

        for name in additional.keys():
            if not isdev(build_type):
                self.config.set(config_name, name, str(uuid.uuid4()))
            else:
                 for mod_var, mod_name in additional.items():
                     self.config.set(config_name, mod_var, mod_name)

        self.config.read(self.config_file)

        self.logger.logger.info('Config %s created:\n' % config_name)
        for i in self.config.items(config_name):
            self.logger.logger.info(i)

        with open(self.config_file, 'w+') as out:
            self.config.write(out)

        self.gitsave(self.config_file, config_name)


    def config_remove(self, config_name):

        """Remove config_name specified in argument from uuids.ini"""

        self.logger.logger.info('Removing %s' % config_name)

        self.config.remove_section(config_name)

        with open(self.config_file, 'w') as out:
            self.config.write(out)

        self.gitsave(self.config_file, config_name)

Usage examples.

Current available configurations:

d:\RDS\rdsmanager>RDSmanager.py unity -l
RDSmanager started at 10, Jul 2015 at 11:01:33
Running configlist

Currently avalable configurations:

develop
debug
qa
version_1_0_2_staging

Add new:

d:\RDS\rdsmanager>RDSmanager.py unity -u test -b ci
RDSmanager started at 10, Jul 2015 at 11:01:56
Running UUID generator with test
WARNING: variable BAMBOO_WORKING_DIRECTORY not found. Will look for RDS_HOME variable...
Using d:\RDS\builddir as workdir

Config test created:

('MATH_DLL', '8546cf29-ca90-4eb0-972b-69f0d7aab0c3')
('MATH_DLL_XML', '2bab667a-1d8d-4811-96b4-f6e81bf9954e')
('ASSETMANAGER_API_DLL', 'f505ddeb-46bc-46a1-9a37-36881afe0b2d')
('ASSETMANAGER_API_DLL_XML', '7ce86064-799e-4aee-be7a-c8dc25210eb5')
...

Remove new:

d:\RDS\rdsmanager>RDSmanager.py unity -x test
RDSmanager started at 10, Jul 2015 at 11:03:00
Removing test
Updating uuids.ini list...
...
Done.
[master a85e51d] Add new configuration set `test`
 1 file changed, 64 deletions(-)
...
\$\endgroup\$
4
\$\begingroup\$

PEP8

This is a style guide for Python that says how code should look.

  • Keep lines to a maximum 79 characters.
    Comments are the only exception at 72.
  • Use snake_case for everything.
    There are a few exceptions. The main one being classes which are CamelCase.

    Just so you know, you are allowed to us full caps acronyms. UUIDManagment is a fine name.

  • Try to limit the amount of empty lines. However.
    • Module level functions and classes should have two blank lines above and below them.
    • Methods should have one blank line above and below them.

Your style is really good otherwise.


PEP257

This is to do with doc-strings.

It doesn't matter wether you have the words on the apostrophes.

"""A multi-line
docstring.
"""

"""
A multi-line
docstring.
"""

Both act in the same way. Personally I think the latter is better, but each to there own.

I would discourage indenting the docstring so that it is not in-line with the apostrophes.

Other than that, they seem good.


Code

'%s' % var is deprecated. Use str.format instead. There are bonuses to using the new format. And less bugs, and edge cases.

raise NoConfigError(self.logger, 'No config file %s found' % self.config_file)
# To
raise NoConfigError(self.logger, 'No config file {} found'.format(self.config_file))

For an example of str.format

>>> '{}, {}, {}'.format('a', 'b', 'c')  # 2.7+ only
'a, b, c'
>>> '{2}, {1}, {0}'.format('a', 'b', 'c')
'c, b, a'
>>> 'Coordinates: {latitude}, {longitude}'.format(latitude='37.24N',
                                                  longitude='-115.81W')
'Coordinates: 37.24N, -115.81W'
>>> "repr() shows quotes: {!r}; str() doesn't: {!s}".format('test1', 'test2')
"repr() shows quotes: 'test1'; str() doesn't: test2"

>>> "int: {0:d};  hex: {0:x};  oct: {0:o};  bin: {0:b}".format(42)
'int: 42;  hex: 2a;  oct: 52;  bin: 101010'

It's quite remarkable, and somehow I learn something new every time I go on the page.


It's normally better to allow Python to handle exits.

except WindowsError as error:
    self.logger.logger.info('ERROR: %s' % error)
    sys.exit(1)

You could at a later-date find out a way to fix this error, or a work around for it. However you will need to remove the sys.exit to do either. It may be better to just raise the error again.

Also it is normally better to have small try statements. If for some reason self.logger raised a WindowsError, you may not handle things the same way.


You use both if mod_tag: and if 'CLOUD' in mod_tag: without their elif or else counterparts.
You can combine them to give you if mod_tag and 'CLOUD' in mod_tag.
In most cases this will be enough. However if mod_tag returns True if there is data in mod_tag. And if 'CLOUD' in mod_tag is a subset of this as 'CLOUD' has to be in the data. And so mod_tag will be true every time 'CLOUD' in mod_tag is.

Also, try to reduce the size of the try statement. If isdev(build_type) raised AttributeError, would you handle this the same way, for example.

mod_tag = parser(self.logger, modname, mod_proj_file)
if 'CLOUD' in mod_tag:
    if isdev(build_type):
        try:
            self.config.set(config_name, modname.upper() + '_DLL', modname + '.dll')
            self.config.set(config_name, modname.upper() + '_DLL_XML', modname + '.dll.xml')
        except AttributeError:
            pass
    else:
        try: 
            self.config.set(config_name, modname.upper() + '_DLL', str(uuid.uuid4()))
            self.config.set(config_name, modname.upper() + '_DLL_XML', str(uuid.uuid4()))
        except AttributeError:
            pass

for name in additional.keys():
    if not isdev(build_type):
        self.config.set(config_name, name, str(uuid.uuid4()))
    else:
         for mod_var, mod_name in additional.items():
             self.config.set(config_name, mod_var, mod_name)

Try to not have nested for loops, unless it is really needed. This can be written to the below, as build_type does not change.

if not isdev(build_type):
    for name in aditional:
        self.config.set(config_name, name, str(uuid.uuid4()))
else:
    for mod_var, mod_name in additional.items():
        self.config.set(config_name, mod_var, mod_name)

Try to not use variables such as i. It's 'ok' when the program is small, however it is not descriptive, and when programs become larger they are a pain.

\$\endgroup\$
  • \$\begingroup\$ Thanks, very useful (but I'm still lazy to investigate str.format() :-( ). Just - could you please clarify this moment: Try to combine if statements. if mod_tag: if 'CLOUD' in mod_tag: with no else. ? \$\endgroup\$ – setevoy Jul 10 '15 at 13:12

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.