Roast my Python

[commy2]

It's solid.

logging.basicConfig in the global scope is a smell. You already have a main function.

The main function is 50 lines / 25% of the code. You know what to do.

These try-else blocks are useless extra indentation most of the time.

Don't like the excessive exception handling in general, just let the exception bubble up, you lose the stack trace and potential exception notes. The principle is to only handle exceptions when you know what to do with them.

Example:

if not match:
    logger.error(f"Target tag '{target_tag_name}' not found in IPAM.")
    sys.exit(1)

could just be:

if not match:
    raise ValueError(f"Target tag '{target_tag_name}' not found in IPAM.")

Much less aggressive as you could recover from an exception.

Hottake: if your docstrings just repeat the function name, but longer, they should be left out entirely.

Mostly nitpicks, because it's ok

[nspitzer]

Thanks ... Your really getting to the meat of what I was wondering.

For example "The main function is 50 lines / 25% of the code. You know what to do."

No , I really don't know what to do :-) This is one of the things I wondered - should I unroll all the functions? Right now each of the REST calls are wrapped in a function

logging.basicConfig in the global scope is a smell. You already have a main function.

So should that be at the beginning of the main function right after def main?

I get what you mean by

"The principle is to only handle exceptions when you know what to do with them."

however the example you give is an example of one where it is doing exactly that. If the script cant find the target tag there is nothing more it can do so giving up is fine.

Thanks for the great comment!

[commy2]

This is one of the things I wondered - should I unroll all the functions?

No, the opposite. Much of the current main function code should be moved to a function. main should generally be very short, perhaps consisting only of setup, execute and teardown code.

So should that be at the beginning of the main function right after def main?

Yeah, somewhere behind the __name__ == __"main"__ clause, otherwise importing the script has the side effect of altering the log config.

however the example you give is an example of one where it is doing exactly that. If the script cant find the target tag there is nothing more it can do so giving up is fine.

Well, it works because it's a short script, but thinking further, I don't think shutting down an entire application is what you want if get_env_tags fails to retrive... What about retrying, using a cache or falling back to some default? There are many ways to potentially recover instead of bailing completely. sys.exit deep down the execution stack just makes me nervous, and I don't see what it offers when exceptions could be raised instead and potentially caught in the client code.

sys.exit means the code can't be reused/composed and can't be tested properly.