-
Notifications
You must be signed in to change notification settings - Fork 34
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
Enum + event constructor for pub result events #194
Conversation
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
/// | ||
/// Note: This will only return surveys that are within the start and end | ||
/// periods of a given survey. |
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.
Unrelated dartdoc update that i wanted to include for my own reference
@@ -56,6 +56,11 @@ enum DashEvent { | |||
description: 'Information for a flutter build invocation', | |||
toolOwner: DashTool.flutterTool, | |||
), | |||
flutterPubResult( | |||
label: 'flutter_pub_result', |
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.
not a blocker for this PR, but just wondering, do you know if long-term we would need this flutter tool event once pub has its own analytics via unified_analytics?
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.
Hmm that's a good question, and i would think it may still be useful to know which pub
subcommands users are running within the flutter context? so yes we can track usage of dart pub get
when we migrate pub, but is it also helpful to track flutter pub get
?... if the answer is no, then this event likely won't be useful
From what I can see, context is just the lists from here
And result is always either "success" or "failure"
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.
I think this is a question for the pub team--I don't know that anyone else uses these numbers.
Note, today we're intentionally suppressing pub's own analytics (https://github.com/christopherfujino/flutter/blob/disable-flutter-build-aar-plugins/packages/flutter_tools/lib/src/dart/pub.dart#L562), so flutter pub ONLY sends Flutter analytics, but I seem to recall that not being our intended final state.
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.
We suppressed the analytics because we were unclear if the pdd covered sending dart analytics from the flutter tool.
I think with the unified analytics all should be good, and we can stop suppressing those, and potentially get actual useful data.
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.
Oh that's good to know, given that we now have the approvals, @christopherfujino are you in favor of deprecating this event? I think there may still be value in tracking successes/failures across different pub invocations from the flutter tool over time... while the pub team can collect more in depth analytics as well on their end.
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.
I am in favor of deleting it, but I will leave the final decision to the pub team, IDK if they use it or potentially would.
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.
Cool, I'll just close this PR for now and check offline. Will reopen if it's still necessary
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.
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
Closing as this event may not be necessary from the flutter tool itself since pub will soon be sending its own analytics from |
Related to the tracker issue:
package:unified_analytics
implementation in flutter tool flutter/flutter#128251Implementing the class below from
flutter/flutter
in this packageContribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.