Configurable server log file. #79
Conversation
tweekmonster
changed the title
|
@blueyed Could you give this a try? |
|
@tweekmonster SGTM, not test yet. |
| ) | ||
| 'deoplete#sources#jedi#show_docstring', False) | ||
| self.debug_server = vars.get( | ||
| 'deoplete#sources#jedi#debug_server', False) |
There was a problem hiding this comment.
What about using True here?
It is only used when generic debug is enabled, and then the server should be included probably by default?
I could imagine that g:deoplete#sources#jedi#debug_server = 1 also enables generic debugging explicitly though - so we would have 3 states here..
There was a problem hiding this comment.
Hmm, yeah probably should be on by default.
I could imagine that g:deoplete#sources#jedi#debug_server = 1 also enables generic debugging explicitly though - so we would have 3 states here..
Could you elaborate?
There was a problem hiding this comment.
With only g:deoplete#sources#jedi#debug_server = 1 debugging should be enabled already - assuming an implicit call to call deoplete#custom#set('jedi', 'debug_enabled', 1).
There was a problem hiding this comment.
That makes sense. This is with the understanding that deoplete#enable_logging() must be called first, right?
| for handler in root_log.handlers: | ||
| if isinstance(handler, logging.FileHandler): | ||
| log_file = getattr(handler, 'baseFilename', None) | ||
| break |
There was a problem hiding this comment.
Can we filter this for the deoplete handler right away?
What happens if no FileHandler is found - in this case the fallback should be logging.FileHandler('/tmp/jedi-server.log') probably still?!
There was a problem hiding this comment.
What do you mean by "right away"? And, if debug_enabled is True, the FileHandler should always be found. I could see the placement being a problem if enabling debug at runtime, though.
There was a problem hiding this comment.
What do you mean by "right away"?
Only consider the "deoplete" root logger.
I could see the placement being a problem if enabling debug at runtime, though.
Yeah, we should consider a feature in the future to change this during runtime.
There was a problem hiding this comment.
Still not following about "Only consider the "deoplete" root logger." Unless I'm misunderstanding, that's what already is happening since the root logger is the only one that has handlers.
TBH, I would rather not scan the handlers and update Deoplete to pass the log location to the source instances, but that requires a third-party change and backward compatibility code (scanning handlers).
There was a problem hiding this comment.
Still not following about "Only consider the "deoplete" root logger."
I've thought that it might be possible to only look at the deoplete root handler right away.
There was a problem hiding this comment.
As in root_logger.handlers[0]?
There was a problem hiding this comment.
Yeah, but in a less hard-coded way.. not sure though.
There was a problem hiding this comment.
Okay, sorry about that. I thought maybe you meant "directly" in the context of class instantiation, or maybe import deoplete.logger.
The reason for scanning is because the only assumption I want to make about Deoplete's logger is that it's named 'deoplete'. I don't want deoplete-jedi to break because logging changed in Deoplete (even though it's not likely). It'll probably be better if the deoplete.logger module had a file_handler variable, but that would require changing Deoplete.
I've noticed more than a few issues in Deoplete where the problem was that Deoplete was out of date or a source was. Not sure how people are updating one and not the other, but I prefer to use the method that's less likely to cause an error.
|
As for the overall PR: I've tried it and it works good already. I've left the comments above afterwards. |
|
@blueyed Updated. I'm going out to dinner. If you feel the update is good enough, you can squash+merge. |
|
I think this is good to go. Merging. |
Closes #74