-
Notifications
You must be signed in to change notification settings - Fork 6
Feature/logging module #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/logging module #24
Conversation
reformatted /home/josh/projects/Cyber-Breach-Detector/Config/config.py reformatted /home/josh/projects/Cyber-Breach-Detector/Common/breach_checker.py reformatted /home/josh/projects/Cyber-Breach-Detector/IP/ip_reputation_checker.py reformatted /home/josh/projects/Cyber-Breach-Detector/main.py reformatted /home/josh/projects/Cyber-Breach-Detector/Common/utils.py reformatted /home/josh/projects/Cyber-Breach-Detector/Email/email_reputation_checker.py reformatted /home/josh/projects/Cyber-Breach-Detector/Username/username_reputation_checker.py All done! ✨ 🍰 ✨ 7 files reformatted, 4 files left unchanged.
note: previous commit didn't go thru
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have encountered some issues while executing these changes in my environment. Apart from that there are some minor issues and typos.
I believe this may have preemptively been merged to main. Apologies if I submitted the review. I was thinking you wouldn't be able to merge to main since I have it as a protected branch.
import sys | ||
import time | ||
|
||
from colorama import Fore, init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this package required?
@@ -1,3 +1,4 @@ | |||
colorama |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version needed, if really required
|
||
# check if the required sections are present | ||
if 'loggers' not in config.sections() or 'handlers' not in config.sections(): | ||
raise ValueError('Missing required section in config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this message would have to be:
raise ValueError('Missing required section in logger.ini')
raise ValueError('Missing required section in config.ini') | ||
|
||
if 'keys' not in config['loggers'] or 'suspicious' not in config['loggers']: | ||
raise ValueError('Missing required key in loggers section in config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar change required:
raise ValueError('Missing required key in loggers section in **logger**.ini')
|
||
# check if the required keys are present in the 'handlers' section | ||
if 'keys' not in config['handlers'] or 'console' not in config['handlers']['keys']: | ||
raise ValueError('Missing required key in handlers section in config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar change required:
raise ValueError('Missing required key in handlers section in **logger**.ini')
|
||
# check if the values of the keys are in the expected format | ||
if not isinstance(config.getint('handlers', 'console.level'), int): | ||
raise ValueError('Invalid value for console.level in config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar change:
raise ValueError('Invalid value for console.level in logger.ini')
raise ValueError('Invalid value for console.level in config.ini') | ||
|
||
if not isinstance(config.getint('loggers', 'keys.suspicious.level'), int): | ||
raise ValueError('Invalid value for keys.suspicious.level in config.ini') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar change:
raise ValueError('Invalid value for keys.suspicious.level in logger.ini')
if not os.path.isdir('Logs'): | ||
os.mkdir('Logs') | ||
if not os.path.exists(os.path.join('Logs', 'traceback.log')): | ||
with open(os.path.join('Logs', 'traceback.log')) as fp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have encountered an issue while running the test when traceback.log file isn't already created. Like in line 10, where Logs directory is created if not exists, we may need to create traceback.log if it doesn't exist.
@GH-Syn The other issue is, when the script is run with the correct API Keys, logs to traceback.log are not being written, and no output is displayed on the terminal. However, no errors during execution. Might have something to do with the file handler and stream handler. Example: $ python main.py -e k@k.cm
$ Don't think a revert of the merge would be recommended, but probably a separate bug issue to resolve these? Let me know your thoughts. |
Update: I have resolved the issue of stream handler not displaying any output in #31. Will rename the issue and push the update |
About this merge
Tasks needed before merge