-
Notifications
You must be signed in to change notification settings - Fork 0
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
logger in module #16
base: main
Are you sure you want to change the base?
logger in module #16
Conversation
@danschmidt5189 In a module file, error messages not exporting to log, it turns out I have to move
|
@yzhoubk Including the Logging module will add the Edit: I just noticed your last comment, which echoes mine. I included the Logging module in the top-level simply because that's where Config (which used to also included Logging logic) was included. Do whatever works best. The DataHandler module has quite a few code smells: why use a module rather than a class, given the state involved? If using a module, why put all the methods into the class block? Etc. But those are bigger than I think you want to handle here. |
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.
This is a good way to solve the immediate problem, so I'm +1 on merging it.
Overall, I think the DataHandler would benefit from being converted into a normal class rather than a module. It carries a lot of state and configuration information which is more naturally expressed via objects. A good goal would be to get it into a state where all of the logic currently expressed in the CLI methods, which handle the order of operations for ingestion, could be folded into this class (allowing us to call the method directly rather than shelling out). That can wait for a future refactoring.
No description provided.