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

Re-use PanLogger instances if the parameters haven't changed. #192

Merged
merged 2 commits into from
Dec 15, 2017

Conversation

jamessynge
Copy link
Contributor

Fixes #190.

@jamessynge jamessynge requested a review from wtgee December 14, 2017 14:16
@jamessynge
Copy link
Contributor Author

It appears the build failed due to a network issue. Closing and reopening PR to trigger build again.

@jamessynge jamessynge closed this Dec 14, 2017
@jamessynge jamessynge reopened this Dec 14, 2017
@codecov
Copy link

codecov bot commented Dec 14, 2017

Codecov Report

Merging #192 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #192      +/-   ##
===========================================
+ Coverage    81.27%   81.32%   +0.05%     
===========================================
  Files           37       37              
  Lines         2654     2662       +8     
  Branches       330      331       +1     
===========================================
+ Hits          2157     2165       +8     
  Misses         392      392              
  Partials       105      105
Impacted Files Coverage Δ
pocs/utils/logger.py 96.82% <100%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1537b05...7451b0b. Read the comment docs.

@wtgee
Copy link
Member

wtgee commented Dec 14, 2017

It appears the build failed due to a network issue. Closing and reopening PR to trigger build again.

FYI, there is a Restart Build button on travis. Let me know if you don't have access to it.

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PanBase there is a global that is making sure this doesn't happen, but that is only for objects that inherit from PanBase. I'm guessing that you ran into this with some of the abstract dome class, which I noticed doesn't inherit from PanBase.

I like the json solution but it seems like it would also just have the problem of creating multiple all_loggers each time it is loaded.

@jamessynge
Copy link
Contributor Author

As long as importing works correctly, all the imports of pocs.utils.logger will reference the same one module instance, so they'll share the same global (module scope) variable all_loggers. And thus a single PanLogger is created.

@wtgee
Copy link
Member

wtgee commented Dec 14, 2017

Let's remove the global _logger and just use this setup. It is essentially the same thing.

@jamessynge
Copy link
Contributor Author

PTAL

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@wtgee wtgee merged commit 2e029dc into panoptes:develop Dec 15, 2017
@jamessynge jamessynge deleted the one_logger branch December 16, 2017 17:51
@wtgee wtgee mentioned this pull request Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants