have app loggers log to their parent logger #10402
Conversation
| AppDomain domain; | ||
| final AppInstance app; | ||
| final bool logToStdout; | ||
| final Logger logToParent; |
| } | ||
| } | ||
|
|
||
| /// A [Logger] which sends log messages to a listening daemon client. |
There was a problem hiding this comment.
I don't really understand this class. What is the logToParent option for? It seems to make the entire class just act like the provided parent, in which case, why bother creating the object at all?
There was a problem hiding this comment.
logToParent changes the behavior to use the parent to handle all stdout. This is valuable for IntelliJ's implementation of the flutter run -v option - previously we weren't passing trace level messages to the IDEs. An equivalent change would be to keep the current behavior - not add logToParent - but change to pass all message levels (including trace) to IDEs.
why bother creating the object at all?
With or without those changes, this class sends progress messages to the IDE, so it can use that info to provide nice, integrated progress UIs.
There was a problem hiding this comment.
Would it make sense to have two subclasses of _AppRunLogger, one for when you want to send all events to the IDE, and one for when you only want to send progress events to the IDE but want status, error, and trace events to instead be logged to the console?
I still don't really understand what this is for.
The actual change (from logToStdout to having an explicit parent) seems like an improvement, I'm just stuck on the underlying code that you're changing here.
There was a problem hiding this comment.
Re: two loggers, that's totally doable - and I do agree that the current state is a bit confusing.
The main value this class adds is letting IDEs convert the spinner progress info available in the CLI to progress mechanisms that make more sense in the IDE (indeterminate progress bars). And what I'm trying to do w/ this change is to plumb more of the verbose info through, so people can use the equivalent of flutter run -v to help diagnose when there are issues launching apps.
The actual change (from logToStdout to having an explicit parent) seems like an improvement
lgtm then, or should I work on specialized _AppRunLogger subclasses?
There was a problem hiding this comment.
You should at a minimum leave a TODO mentioning that it might make sense to refactor this code.
| <excludeFolder url="file://$MODULE_DIR$/test/runner/packages" /> | ||
| <excludeFolder url="file://$MODULE_DIR$/test/src/packages" /> | ||
| <excludeFolder url="file://$MODULE_DIR$/tool/packages" /> | ||
| </content> |
There was a problem hiding this comment.
They were, so I'd have less scm churn here. Note that the not-yet-released version of IntelliJ won't generate these excludes, so there won't be as many changes to these files.
There was a problem hiding this comment.
Can we do this in a separate PR maybe? I don't really understand why this particular file exists in our repo at all, but that's probably a discussion for another bug.
|
LGTM but @tvolkert is the right person to review this. |
|
Sounds good (and added a TODO). |
|
LGTM modulo @Hixie's comment about |
|
Sure, reverted the .iml file change. |
4 participants
-v)@tvolkert