-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add terminal info to flutter command event #204
Add terminal info to flutter command event #204
Conversation
@leighajarett is it actually necessary to have this field as a user property? After looking through flutter code, we are only sending this along with each command event It looks like the only two fields we send with each event is for the channel and OS, which we are currently doing now |
int? maxRss, | ||
}) : eventName = DashEvent.flutterCommandResult, | ||
eventData = { | ||
'commandPath': commandPath, | ||
'result': result, | ||
'commandHasTerminal': commandHasTerminal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related to tracker issue:
package:unified_analytics
implementation in flutter tool flutter/flutter#128251Closes:
While migrating the
Usage.command
static method, includes thecommandHasTerminal
parameter, I realized that it probably makes sense to include it with the previously migratedCommandResult
event.This new event will now let us record which flutter command was run, while also keeping track of whether the invocation included a terminal.
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.