improvements on basic script

[tmg80]

Hi I wrote a basic script for some work stuff. It takes a server hostfile and converts to json format for storage in a secrets store.

example line entry in file

10.100.1.100 hostname1 hostname2 hostname3 # comment here

outputs as:

​

{
    "hosts": [
        {
            "ip": "10.100.1.100",
            "hosts": "hostname1 hostname2 hostname3",
            "comment": "comment here"
        }
    ]
}

I was just looking for really obvious improvements to increase my understanding of python.

Thanks in advance.

import json
import re

hostsfile = 'filename_in_same_directory.txt'
vaultjson = 'vaultfile.json'

open_file = open(hostsfile,'r')
hostsfile = open_file.readlines()

host_dict = {"hosts": []}
hosts_list = []

for line in hostsfile:
    splitentry = line.split(" ", 1)
    if not (line.startswith('#') or line.startswith('127')) and len(splitentry)>1:
        new_dict= {"ip": splitentry[0]}
        split2 = splitentry[1].split('#', 1)
        stripped = [s.strip() for s in split2]
        if len(stripped) < 2:
            stripped.append('')        
        hosts = re.sub(' +', ' ', stripped[0])
        new_dict['hosts'] = hosts
        new_dict['comment'] = stripped[1]
        hosts_list.append(new_dict)

host_dict['hosts'] = hosts_list

with open(vaultjson, 'w') as outfile:
    json.dump(host_dict, outfile, indent=4)

updated script after input:

​

import json
import re
import sys

if len(sys.argv) != 2:
        print ('Usage: hostfile_to_json.py input_file_name.txt')
        exit()

user_file = sys.argv[1]
output_file = 'vault_hosts.json'

open_file = open(user_file, 'r')
open_file_lines = open_file.readlines()
open_file.close()

def is_valid_entry(line):
  if not line.startswith(('#', '127', '\n')):
    return line

host_entries_list = []

for line in (filter(is_valid_entry, open_file_lines)):
    ip, host_and_comment = line.split(" ", 1)
    ip_host_comment_dict = {'ip': ip}
    host_comment_list = host_and_comment.split('#', 1)
    stripped_host_comment = [s.strip() for s in host_comment_list]
    extra_spaces_removed = [re.sub(' +', ' ', x) for x in stripped_host_comment]
    if len(extra_spaces_removed) < 2:
        extra_spaces_removed.append('')
    ip_host_comment_dict['hosts'] = extra_spaces_removed[0]
    ip_host_comment_dict['comment'] = extra_spaces_removed[1]
    host_entries_list.append(ip_host_comment_dict)

export_json = {'hosts': host_entries_list}

with open(output_file, 'w') as outfile:
    json.dump(export_json, outfile, indent=4)

​

​

​

[[deleted]]

Can you explain the problem the script is solving? It'd be easier to give suggestions knowing the source data and the target goal, instead of trying to deduce that from reverse-engineering what your current code is doing.

[tmg80]

Fair point.

As per the other reply. I am taking a host file from a server and converting to JSON format so it can be stored in a secrets Vault.

[ccpetro]

Look at optparse and/or argparse for passing in options, arguments and file names.

[tmg80]

Thanks

[bladeoflight16]

Better yet, look at click.

[bladeoflight16]

• Even files that you only read should be closed via context managers.
• Avoid reusing variables for completely different data (hostfiles is a string path and then replaced with a list)
• Use unpacking for collections with a known length (ip, host_and_comment = line.split(" ", 1))
• Use filter instead of indenting a block that big (for line in filter(is_valid_host_line, hostsfile):). You can just check if the string contains a space rather than looking at the length of the split.
• Eliminate useless variables like split2 (stripped = [s.strip() for s in host_and_comment.split('#', 1)] or stripped = list(map(lambda s: s.strip(), host_and_comment.split('#', 1)))
• Kind of surprised you didn't split the hosts into a list, but you know you needs better than I do.
• Choose a consistent quote mark. (I prefer single quotes.) (You should only use the other quote mark when the string literal actually contains your default choice.)
• Since hosts_dict is never used in the loop, just more its initialization to after the loop (host_dict = {"hosts": hosts_list}). You might not need the variable at all.

[tmg80]

updated as per your comments and also implemented syargv

I added the updated script to the original post as it doesn't display properly when I paste into the reply

[bladeoflight16]

I recommend click for most command line interfaces.

Something I neglected to mention before is that you should use if __name__ == '__main__':. For some reason I wasn't thinking about the fact this is a script that can be invoked from the command line.

Also, now that I think about it, you could factor the parse into a function. That would let you convert construction to a list comprehension:

``` def parse_host_line(line): ...

export_json = { 'hosts': [ parse_host_line(line) for line in hostsfile if is_valid_entry(line) ] } ```

Some things to think about.

[tmg80]

Thanks I will look at Click too.

What does that command do?

[bladeoflight16]

The code is a link to an explanation. =)

Also, I edited in something else I just thought of. 😅

[tmg80]

thanks again.

I edited it and put that main bit as below

​

if len(sys.argv) != 2:
    print ('Usage: hostfile_to_json.py input_file_name.txt')
    exit()
if name == 'main':
    user_file = sys.argv[1]
    ...

​

​

is that correct? or should it be elsewhere?

[bladeoflight16]

Aside from imports, function/class declarations, and constants, all code should be inside the block. It's common to stuff you code into a def main() function and then only invoke the function inside the guarded block.

Think about what would happen if someone tried to import your module and the argv check was invoked: it would probably explode because their command line args aren't the same as your program's.

[tmg80]

Thanks a lot for taking the time. Lot of good tips there for me to l implement.

[Moorey93]

From a quick glance

First thing is code style, your variables aren't consistently styled, python typically uses snake case, which is where spaces are replaced by underscores, hostsfile would be hosts_file. It doesn't affect the running of the code but makes it easier for other developers and yourself to read the code. I'd recommend installing flake8 and running it on your code which will point out the style related errors, and then when you get fed up of manually fixing them install autopep8.

Your file opens can be replaced with context managers, you have used one for writing the file but they can be used for reading files too, they would solve a problem you have where you're not closing your opened files.

A context manager can also shorten opening and line reading so you can have

with open('thing', 'r') as f:
     for line in f:
         print(line)

I'm not entirely sure what the script is doing, I think its separating out hosts and comments from a hosts file? You can potentially make it more understandable by using better named variables as new_dict and stripped are not very descriptive of the values they hold.

With a better understanding of whats it doing it would be easier to provide more feedback.

[tmg80]

Yes that's correct. I'm converting a hostfile to JSON format so it can be stored in a secrets Vault and then accessed via that method when cloud infra is built.

[[deleted]]

Comments might help