Clean up low-hanging fruit. #11
Conversation
|
Looks like you know what you're doing, I'll have to check it out and test it Thanks a lot. Are you in the discord? |
|
Not in the Discord, no. I'll join it right now. |
|
Joined, this is my Discord ID 427783182948106241 |
| socket.gaierror, | ||
| ) as error_message: | ||
| print("Connection Error") | ||
| print(error_message) |
There was a problem hiding this comment.
could this be done better with the python stdlib logging module?
There was a problem hiding this comment.
I don't like Python's logging module. Maybe something like https://github.com/Delgan/loguru?
There was a problem hiding this comment.
What's wrong with the logging module? It's in stdlib and it's mostly the same as prints but more organised when in default settings
There was a problem hiding this comment.
I've found https://github.com/Delgan/loguru to be much more pleasing to use, I think the stdlib logging requires more configuration. To be honest both are suitable for this, loguru is just what I prefer
| url += "/api/aircraft/v2/all" | ||
| else: | ||
| raise ValueError("Proxy enabled but no host") | ||
| headers = {"api-auth": API_KEY, "Accept-Encoding": "gzip"} |
There was a problem hiding this comment.
adding some whitespace in the below code could make it more readable
| from datetime import datetime | ||
| import pytz | ||
| import os | ||
| import signal | ||
| import sys | ||
| import requests | ||
| from zipfile import ZipFile |
There was a problem hiding this comment.
could these imports follow PEP8
features
stdlib
external
relative
style imports?
There was a problem hiding this comment.
Totally agree! Actually didn't know that this was in PEP8. Nice to see it's formalized.
| f.write(file_content.content) | ||
|
|
||
| except Exception as e: | ||
| raise e(f"Error getting{file_name} from {url}") |
There was a problem hiding this comment.
codacy says e is not callable and it is correct, it might help to either use
raise type(e)(msg) from eor the better way is
raise CustomException(...) from e| import platform | ||
| import requests | ||
| import json | ||
| import re |
There was a problem hiding this comment.
and the imports thing but with all files perhaps
|
|
||
| try: | ||
| photo_box = BROWSER.find_element_by_id("silhouette") | ||
| except: |
There was a problem hiding this comment.
the try except pass way of doing things is absolutely awful, either show the error to console or use except ErrorType to be more precise with what you expect
| print(self) | ||
| # Ability to Remove old Map | ||
| import os | ||
| from colorama import Fore, Style |
There was a problem hiding this comment.
whats with the re-imports and localised imports? theres no circular imports as they are not relative
https://github.com/Jxck-S/plane-notify/pull/11/files#diff-dff727939aab7b094d4de98bb64ca4b1c86c99592f797e6a88f682843bd246c0R8
| if os.path.isfile("lookup_route.py") and ( | ||
| self.db_flags is None or not self.db_flags & 1 | ||
| ): | ||
| from lookup_route import lookup_route |
There was a problem hiding this comment.
this import looks so out of place and is unused, same with many others
| hours, remainder = divmod(elapsed_time.total_seconds(), 3600) | ||
| minutes, seconds = divmod(remainder, 60) | ||
| print( | ||
| ( |
There was a problem hiding this comment.
is this second bracket pair necessary?
| @@ -0,0 +1,107 @@ | |||
| import requests | |||
| import json | |||
| import configparser | |||
Did a ton of refactors including reformatting to follow common formatting rules, unused variables, unsorted imports, etc.