more pythonic way

[fstefanov]

Hello all,

I'm trying to improve my python skills and
I made one small script and like to share it with you guys
If someone can give me some remarks how to improve it and
make it more Pythonic I'm eager to learn.

#!/usr/bin/env python2

import sys
import os
import json
sys.path.insert(0, './libs')
from artifactory import ArtifactoryPath, md5sum, _ArtifactoryAccessor
# cannot post clickable links ...
SERVER = '192.168.10.2'
URL = 'localhost:8081/artifactory'
get_class = _ArtifactoryAccessor()

print('\n')
print('=' * 80)
print("Downloading APKs from %s" % SERVER)
print('=' * 80)
print('\n')

def download_artifact(url):
    path = ArtifactoryPath(url)
    try:
        with path.open() as fd:
            with open(LOCAL_PATH, "wb") as out:
                out.write(fd.read())
    except (RuntimeError, OSError) as err:
        return 1
    else:
        local_md5sum = md5sum(LOCAL_PATH)
        compare_checksum(url, local_md5sum)

def compare_checksum(url, local_md5sum):
    try:
        get_stat = get_class.rest_get(url)
        remote_md5sum = get_stat[0]['X-Checksum-Md5']
        # using OSError here not enough in python2
    except Exception as err:
        sys.exit(("Cannot connect to %s") % SERVER)
    else:
        if local_md5sum == remote_md5sum:
            print(("STATUS: %s -OK-") % ARTIFACT_ID)
        else:
            print("MD5checksum does not match downloading again")
            download_artifact(url)

with open('repos.json') as json_data:
    d = json.load(json_data)

for a, repos in enumerate(d['REPO']):
    REPO = repos['name']
    for b, groups in enumerate(d['REPO'][a]['GROUP_ID']):
        GROUP_ID = groups['name']
        for artifacts in d['REPO'][a]['GROUP_ID'][b]['ARTIFACT_ID']:
            ARTIFACT_ID = artifacts['name']
            RELEASE = artifacts['version']
            PATH = artifacts['path']
            TYPE = artifacts['type']
            
            ARTIFACTORY_PATH = "%s/%s/%s/%s/%s/%s-%s.%s" % (
                URL, REPO, GROUP_ID, ARTIFACT_ID, RELEASE, ARTIFACT_ID, RELEASE, TYPE)
            ARTIFACTORY_PATH_RELEASE = "%s/%s/%s/%s/[RELEASE]/%s-[RELEASE].%s" % (
                URL, REPO, GROUP_ID, ARTIFACT_ID, ARTIFACT_ID, TYPE)
            LOCAL_PATH = "/%s/%s.%s" % (PATH, ARTIFACT_ID, TYPE)

            try:
                local_md5sum = md5sum(LOCAL_PATH)
            except IOError as ioex:
                print("Downloading: %s.%s" % (ARTIFACT_ID, TYPE))
                if download_artifact(ARTIFACTORY_PATH):
                    print("WARNING:", ARTIFACTORY_PATH, "Not Found")
                    print("Trying latest available version...")
                    if download_artifact(ARTIFACTORY_PATH_RELEASE):
                        sys.exit("ERROR: Cannot Download Artifact Aborting !!!")
            else:
                compare_checksum(ARTIFACTORY_PATH, local_md5sum)
                        

print('\n')
print('=' * 80)
print('Downloading DONE')
print('=' * 80)
print('\n')

[ichabod801]

Your download_artifact function explicitly returns a 1 on an error and implicitly returns None on success. I think it should explicitly return in both cases, and I think it should return False for an error and True otherwise. You would then test on if not download_artifact(path):

Single letter variable names are not pythonic.

I think the double for loop at the end should go into a function, one that takes the data from json as a parameter. That section is full of module level variables. Module level variables should be constants, and should be limited as much as possible. Then put the remaining global code (the prints, the data loading, and calling the new function) in an if __name__ == '__main__': block. Then it will only run if the module is the top level module. It won't run if the module is just imported. Global level code should be as limited as possible.

I would add more comments, especially at the start of your functions. It is traditional to put a triple quoted comment at the beginning of each function, method, or class. This automatically becomes the docstring of that object, which can be returned in the interpreter with help(object). The same goes for the module as a whole.

[volcano63]

My 2 cents - all-uppercase names are traditionally used for constants - not that - strictly speaking, we have them in Python, but it's a convention. Usually you assign those at the module start, and within functions - only referenced. Not assigned.

[Larz60+]

Also Camel Case should be used for class names, but not for variable, function or method names
PEP8 rules here: https://www.python.org/dev/peps/pep-0008/

[buran]

I would use new string formatting, instead of old-style.

[Larz60+]

I would use new string formatting, instead of old-style.

Do you mean with .format or then python 3.6 style

print(f'{place} is an {adjective} place to visit.')

I like the 3.6 style, but have been a bit reluctant to use it in my scripts,
I suppose it's one way to get users to upgrade.

[ichabod801]

I would say .format. I don't think 3.6 is in wide use just yet. (And you're missing a quote)

[buran]

Do you mean with .format or then python 3.6 style

To be honest, i meant the .format style. Not yet used to 3.6 f-strings myself :-)

[fstefanov]

Great, thank you all !!!
This is my new version for now.

sys.path.insert(0, './libs')
from artifactory import ArtifactoryPath, md5sum, _ArtifactoryAccessor

server = '192.168.10.2'
server_url = 'http://{}/artifactory'.format(server)
apk_file='apk_list.json'
get_class = _ArtifactoryAccessor()

def main(d):
    print('\n')
    print('=' * 80)
    print('Downloading APKs from {}'.format(server))
    print('=' * 80)
    print('\n')

    for a, repos in enumerate(d['REPO']):
        repo = repos['name']
        for b, groups in enumerate(d['REPO'][a]['GROUP_ID']):
            group_id = groups['name']
            for artifacts in d['REPO'][a]['GROUP_ID'][b]['ARTIFACT_ID']:
                artifact_id = artifacts['name']
                release = artifacts['version']
                path = artifacts['path']
                file_type = artifacts['type']
                
                artifactory_path = '{}/{}/{}/{}/{}/{}-{}.{}'.format(
                    server_url, repo, group_id, artifact_id, release, artifact_id, release, file_type)
                artifactory_path_release = '{}/{}/{}/{}/[RELEASE]/{}-[RELEASE].{}'.format(
                    server_url, repo, group_id, artifact_id, artifact_id, file_type)
                local_path = '/{}/{}.{}'.format(path, artifact_id, file_type)

                try:
                    local_md5sum = md5sum(local_path)
                except IOError as ioex:
                    print('Downloading: {}.{}'.format(artifact_id, file_type))
                    if not download_artifact(artifactory_path, local_path, artifact_id):
                        print("WARNING:", artifactory_path, "Not Found")
                        print("Trying latest available version...")
                        if not download_artifact(artifactory_path_release, local_path, artifact_id):
                            sys.exit("ERROR: Cannot Download Artifact Aborting !!!")
                else:
                    if not compare_checksum(artifactory_path, local_md5sum, artifact_id):
                        print("WARNING:", artifactory_path, "Not Found")
                        print("Trying latest available version...")
                        if not compare_checksum(artifactory_path_release, local_md5sum, artifact_id):
                            sys.exit('Cannot connect to {}'.format(server))
    print('\n')
    print('=' * 80)
    print('Downloading DONE')
    print('=' * 80)
    print('\n')
    
def download_artifact(url, local_path, artifact_id):
    path = ArtifactoryPath(url)
    try:
        with path.open() as fd:
            with open(local_path, "wb") as out:
                out.write(fd.read())
    except (RuntimeError, OSError) as err:
        return False
    else:
        local_md5sum = md5sum(local_path)
        compare_checksum(url, local_md5sum, artifact_id)
        return True

def compare_checksum(url, local_md5sum, artifact_id):
    try:
        get_stat = get_class.rest_get(url)
        remote_md5sum = get_stat[0]['X-Checksum-Md5']
        # using OSError here not enough in python2
    except Exception as err:
        return False
    else:
        if local_md5sum == remote_md5sum:
            print('STATUS: {} -OK-'.format(artifact_id))
            return True
        else:
            print("MD5checksum does not match downloading again")
            download_artifact(url)
    
if __name__ == "__main__":
    try:
        with open(apk_file) as json_data:
            data = json.load(json_data)
            main(data)
    except IOError as ioerr:
        sys.exit('{} File not found'.format(apk_file))

I will add comments and docstings later on :)
I was thinking should i use kwargs for passing parameters
and can i concatenate those beginning and end prints.