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

Juched78 log file fix #48

Merged
merged 5 commits into from
Jul 8, 2016
Merged

Conversation

rct
Copy link
Collaborator

@rct rct commented Jul 4, 2016

  • Moved @juched78's tornado log to file fix from a zip file attached to issue Logging in Tornado version doesn't write to file #36 to a branch/PR so the changes can be easily evaluated and/or merged.
  • These changes are all from @juched78, this fix required some minor structural changes to complete configuration before setting up the logger.
  • Since I didn't have a clean diff, the first commit adds @juched78's changes, which would have overwritten the plugin loading fix from Feature/tornado #42. The second commit fixes that. So this should now be a fairly clean patch other than the old bits that were commented out that should eventually be deleted.

rct added 2 commits July 4, 2016 13:14
* Moved @juched78's tornado log to file fix from a zip file
  attached to issue juggie#36 to a branch/PR so the changes
  can be easily evaluated and/or merged.
(When copying in juched78's files I didn't catch that it
would have overwitten the plugin load fix from juggie#42.)
@juggie
Copy link
Owner

juggie commented Jul 5, 2016

Could this be cleaned up to not have commented out code all over the place.

@rct
Copy link
Collaborator Author

rct commented Jul 5, 2016

Will do. My intent was just to take @juched78's fixes out of the zip and turn into a diff/branch for testing/evaluation.

@juggie
Copy link
Owner

juggie commented Jul 5, 2016

Looks much better but at quick glance I am not 100% sure why the logger.debug calls are being replaced with print() in 2 locations. I'll take a closer look shortly.

@rct
Copy link
Collaborator Author

rct commented Jul 6, 2016

Looks like he replaced those 2 lines because logger doesn't get imported until after config is done.

Though I just noticed that the check and exit if the config file didn't get loaded was removed.

@rct
Copy link
Collaborator Author

rct commented Jul 6, 2016

While this patch had removed the exit if config file not found, I don't think those lines worked. So I've added a commit to raise an error if the config file hasn't been processed. There are enough config defaults that things will sort of run without a config file. However it seems more appropriate, given the current setup to raise an error (which will force an exit).

* found hopefully the last bit of commented out code from
  this patch.
@juggie
Copy link
Owner

juggie commented Jul 7, 2016

We don't really need logger to know how to talk to the config module, we can easily have the logger be initialized in the main and tell it what kind of logging we want as part of application start up.

@rct
Copy link
Collaborator Author

rct commented Jul 7, 2016

I didn't really want to take ownership of this patch, but I did want logging to a file to work. My point of view on this PR at the moment, knowing very little about the Python logger, is that it is fairly reasonable at this point. Yes there have been 2-3 logger.debugs that have been replaced with prints because they are executed before the logger is configured.

I haven't had a chance to figure out what would be needed, if anything, to start up the logger in a default stderr config and then reconfigure it for file logging later. Sure it would be good for cleanliness, but is low on my priority list at the moment.

@juggie juggie merged commit f937246 into juggie:feature/tornado Jul 8, 2016
@rct rct deleted the juched78-log-file-fix branch July 8, 2016 14:14
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