-
Notifications
You must be signed in to change notification settings - Fork 0
ioana.circu - logging #4
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
Conversation
- setup function in separate file - added option to not log the output if required - added extra two lines in dirconfig for the log file location
@@ -0,0 +1,45 @@ | |||
import logging | |||
|
|||
def setup_logging(enable_logging=True, console_logging=True): |
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.
Adding this comment here but it can apply in many places. Consider using type hints for the parameters of functions. In this case enable_logging: bool = True
etc.
If multiple variable types are acceptable, you can use the following:
from typing import Union
...
enable_logging: Union[bool,None] = None
flightpipe/logger.py
Outdated
:param enable_logging: Flag to enable/disable logging. | ||
""" | ||
|
||
f = open('dirconfig','r') |
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.
Can use the updated syntax for opening files:
with open(file) as f: # 'r' is default if not specified.
content = [r.strip() for r in f.readlines()] # Removes the '\n' from all lines
|
||
settings_default = { | ||
"hosts": [ | ||
"https://es10.ceda.ac.uk:9200" |
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.
Can just use elasticsearc.ceda.ac.uk
now.
|
||
self.es = Elasticsearch(**connection_kwargs) | ||
|
||
def preprocess_records(self, records): |
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.
Look into python Abstract Base Classes at https://www.geeksforgeeks.org/abstract-classes-in-python/
This is something Dave suggested but I know less about. The preprocess_records function is overridden by subclasses (not sure where), which is exactly the use case for an ABC class.
@@ -22,6 +25,7 @@ | |||
urllib3.disable_warnings() | |||
|
|||
def resolve_link(path, ): | |||
logger.debug("Debug: Resolving link for path %s", path) |
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.
Don't need the Debug:
part of the string if your log message already adds this.
Few additional comments for suggested changes to other parts of the code:
Along with a copy of the License file from another project that sits in the top of the repository. |
Great, looks like all the logging changes are fine and working so you can merge this branch into main now! |
Added logger