Skip to content
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

Move logger management into new functions #719

Closed
tsalo opened this issue Apr 16, 2021 · 2 comments · Fixed by #750
Closed

Move logger management into new functions #719

tsalo opened this issue Apr 16, 2021 · 2 comments · Fixed by #750
Labels
effort: low Theoretically less than a day's work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent refactoring issues proposing/requesting changes to the code which do not impact behavior

Comments

@tsalo
Copy link
Member

tsalo commented Apr 16, 2021

Summary

We can move the logger management for the general, report, and reference loggers into new utility functions. Perhaps something like tedana.io.setup_loggers and tedana.io.teardown_loggers.

Additional Detail

Two benefits are (1) less duplicated code between the two workflows and (2) the workflow functions will be more focused on the actual workflows.

Next Steps

  1. Move logger creation-related code into a new function.
  2. Move logger deconstruction-related code into a new function.
  3. Use both functions in both tedana.workflows.tedana_workflow and tedana.workflows.t2smap_workflow.
@tsalo tsalo added the refactoring issues proposing/requesting changes to the code which do not impact behavior label Apr 16, 2021
@jbteves
Copy link
Collaborator

jbteves commented Apr 19, 2021

Do we want to include the loggers in the OutputGenerator class?

@tsalo
Copy link
Member Author

tsalo commented Apr 19, 2021

I don't think it's necessary. We can close all of the loggers' handlers just using the loggers themselves. Plus I don't think we want to pass our OutputGenerator to every function that involves logging.

The part I'm currently stuck on is ensuring that the general logger (LGR) is in fact the same across modules. As it stands, it's almost like we have a completely different LGR for each module, and the only way to operate on all of those at once is through logging.root, which may not be optimal. I'm looking into using LGR = logging.getLogger("GENERAL") instead, while still retaining the logging format we like.

@tsalo tsalo added effort: low Theoretically less than a day's work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent labels Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: low Theoretically less than a day's work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent refactoring issues proposing/requesting changes to the code which do not impact behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants