Create file path and filename in JSON and send it to Discord

Question

I have been working on a function which I call report. What it does is that whenever I call the report function. I can give it a payload of my dict with some data and as well as reason where I can have custom message or the exception errors of whatever can happened.

For now I have created something simple where I create names for file_path and filename. I check if there is a path for that name already and if not then we create. We create a JSON file with the report data to my local PC and then I send it to discord. Both the JSON file and a small text that describes that there has been and error and to check it out ASAP.

My question here is pretty simple to see if there is anything I can actually improve my code somehow. Make it less code or a better report, I take whatever possible!

#!/usr/bin/python3
# -*- coding: utf-8 -*-
import json
import os
import time
from datetime import datetime
from typing import List
from discord_webhook import DiscordEmbed, DiscordWebhook
# payload example payload = {"store": "burger-king"}
# reason = Custom message or exception err
def report(payload=None, reason=None):
 webhook = DiscordWebhook(
 url="https://discord....."
 )
 embed = DiscordEmbed(title="An error has occurred", color=16711680)
 # -------------------------------------------------------------------------
 # Create file path and filename
 # -------------------------------------------------------------------------
 file_path = f'./error/{payload["store"] if payload else "Other"}'
 filename = f'{datetime.strftime(datetime.now(), "%Y-%m-%d_%H-%M-%S-%f")}.json'
 # Check if the path already exists
 if not os.path.exists(file_path):
 os.makedirs(file_path)
 # Write to a JSON file
 with open(f"{file_path}/{filename}", "w", encoding="utf-8") as f:
 f.write(json.dumps(
 {
 "payload": payload if payload else None,
 "reason": str(reason)
 },
 indent=4,
 ensure_ascii=False
 ))
 
 # -------------------------------------------------------------------------
 # Filename embed
 # -------------------------------------------------------------------------
 embed.add_embed_field(name="Filename", value=filename, inline=False)
 # -------------------------------------------------------------------------
 # Send files to discord
 # -------------------------------------------------------------------------
 with open(f"{file_path}/{filename}", "rb") as f:
 webhook.add_file(file=f.read(), filename=f"{filename}")
 # -------------------------------------------------------------------------
 # Footer timestamp
 # -------------------------------------------------------------------------
 embed.set_footer(text=f'AutoSnkr | {datetime.now().strftime("%Y-%m-%d [%H:%M:%S.%f")[:-3]}]')
 webhook.add_embed(embed)
 # -------------------------------------------------------------------------
 # Send to discord with exceptions
 # -------------------------------------------------------------------------
 while True:
 response = webhook.execute()
 
 # Workaround bug from discord_webhook pypi
 if isinstance(response, List):
 assert (len(response) == 1)
 response = response[0]
 
 # Successful requests
 if response.ok:
 return
 
 # Rate limit. Wait until the limitation is gone
 elif response.status_code == 429:
 sleep_time = int(response.headers["retry-after"]) / 1000
 time.sleep(sleep_time)
 else:
 print("Big error here! This text needs to be replaced later on")
 return

Answer 1

• There's comments above report which should really be a docstring, since they explain the function's workings.
• The default parameters for report are a bit suspect:
  • The expression payload if payload else None is redundant, can just be payload then.
  • str(reason) for the default value None for reason is odd, maybe that also needs special casing and could be left out of the JSON body?
• Does the value from payload["store"] need escaping / is it safe? E.g. if it's coming from the server you'd have to validate it before using it as part of a filename to prevent attacks.
• I feel it's a bit over-commented. Consider "Create file path and filename" - that's basically what I can discern from the block below, same as "Check if the path already exists", that's basically the same as the line if not os.path.exists(file_path). Consider splitting off smaller functions that have descriptive names instead of one huge function, that way you wouldn't have to comment on blocks of code (and would instead use the function names as descriptors).
• with is used, good.
• os.makedirs won't do anything if the path already exists, so the check before it is redundant.
• Consider making webhook and embed (global) constants, they otherwise don't seem to add anything to the function.
• After writing the JSON to the file, it's immediately being read back for the webhook.add_file - avoid that unnecessary computation and reuse the in-memory representation of the data.
• Again, consider splitting the setup phase and the while True loop into separate functions to help understanding and, potentially, testability.
• The only thing I'd have to complain about in the loop is the fetching of the retry timeout value: Be conservative in what you expect. The server doesn't have to send that value and if it doesn't you'll get exceptions. Consider providing a default value and a maximum sleep amount to not open yourself up to a malicious server (yes, that's a stretch, but still).
• Just noticed: You're importing typing.List just to check isinstance - AFAIK you can just use isinstance(response, list) without the need to import the typing module. Of course using typing is great, you'd just have annotate your functions most likely and use mypy or a similar type-checker ...

• PythonNewbie: Appreciate the beautiful code review! There was alot of new stuff I wouldn't think of and will of course add it to my code! Thank you so much for it!