-
Notifications
You must be signed in to change notification settings - Fork 78
Add new Event.idePluginEvent
#2229
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
base: main
Are you sure you want to change the base?
Conversation
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
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.
Code Review
This pull request introduces a new Event.idePluginEvent for analytics from IDE plugins, which is a great addition. The implementation is clear, follows existing patterns, and is accompanied by a new test case. My main concern is that a maintenance test that verifies constructor coverage (Confirm all constructors were checked in event_test.dart) seems to have been missed. It needs to be updated to account for the new constructor to prevent test failures. I've also pointed out a minor typo in a description string.
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/unified_analytics/lib/src/constants.dart | 💔 Not covered |
| pkgs/unified_analytics/lib/src/enums.dart | 💚 100 % |
| pkgs/unified_analytics/lib/src/event.dart | 💚 98 % ⬆️ 0 % |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ⚠️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| unified_analytics | Condition | survey_handler.dart::Survey::conditionList survey_handler.dart::Survey::new::conditionList |
| unified_analytics | PersistedSurvey | survey_handler.dart::SurveyHandler::fetchPersistedSurveys |
| unified_analytics | GAClient | analytics.dart::Analytics::fake::gaClient analytics.dart::AnalyticsImpl::new::gaClient |
| unified_analytics | UserProperty | analytics.dart::FakeAnalytics::userProperty |
This check can be disabled by tagging the PR with skip-leaking-check.
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Unrelated files missing license headers
| Files |
|---|
| pkgs/bazel_worker/benchmark/benchmark.dart |
| pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart |
| pkgs/boolean_selector/example/example.dart |
| pkgs/clock/lib/clock.dart |
| pkgs/clock/lib/src/clock.dart |
| pkgs/clock/lib/src/default.dart |
| pkgs/clock/lib/src/stopwatch.dart |
| pkgs/clock/lib/src/utils.dart |
| pkgs/clock/test/clock_test.dart |
| pkgs/clock/test/default_test.dart |
| pkgs/clock/test/stopwatch_test.dart |
| pkgs/clock/test/utils.dart |
| pkgs/coverage/lib/src/coverage_options.dart |
| pkgs/html/example/main.dart |
| pkgs/html/lib/dom.dart |
| pkgs/html/lib/dom_parsing.dart |
| pkgs/html/lib/html_escape.dart |
| pkgs/html/lib/parser.dart |
| pkgs/html/lib/src/constants.dart |
| pkgs/html/lib/src/encoding_parser.dart |
| pkgs/html/lib/src/html_input_stream.dart |
| pkgs/html/lib/src/list_proxy.dart |
| pkgs/html/lib/src/query_selector.dart |
| pkgs/html/lib/src/token.dart |
| pkgs/html/lib/src/tokenizer.dart |
| pkgs/html/lib/src/treebuilder.dart |
| pkgs/html/lib/src/utils.dart |
| pkgs/html/test/dom_test.dart |
| pkgs/html/test/parser_feature_test.dart |
| pkgs/html/test/parser_test.dart |
| pkgs/html/test/query_selector_test.dart |
| pkgs/html/test/selectors/level1_baseline_test.dart |
| pkgs/html/test/selectors/level1_lib.dart |
| pkgs/html/test/selectors/selectors.dart |
| pkgs/html/test/support.dart |
| pkgs/html/test/tokenizer_test.dart |
| pkgs/html/test/trie_test.dart |
| pkgs/html/tool/generate_trie.dart |
| pkgs/pubspec_parse/test/git_uri_test.dart |
| pkgs/stack_trace/example/example.dart |
| pkgs/watcher/test/custom_watcher_factory_test.dart |
| pkgs/yaml_edit/example/example.dart |
This check can be disabled by tagging the PR with skip-license-check.
bwilkerson
left a comment
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 don't think it's necessary to update this package because none of the code in it will actually be invoked. Curious to know what your rationale was. If I'm right, then the good news is that you won't need to update or version this package in order to update the analytics being sent from the plugins.
It looks like the IDE enum is only being used to pass in an event name. I don't think that will be necessary when you write this in Java for IntelliJ based IDEs and in TypeScript for VS Code based IDEs; you can just hard code it to the one appropriate choice.
|
I forgot to mention, but I suspect you're going to want to add a field (outside of |
My thinking is that IDEs will communicate with UA via DTD and that we'll want to update DTD to create and send these new events. |
That's what I imagined also. I don't think we want to communicate with UA ourselves when we can use DTD. This formatting of data also seems reasonable to me. I'm curious, what happens to all of the things inside |
|
Interesting. That does take care of the need to duplicate the package (twice). But I do wonder about the security implications. Will every DevTools extension to be able to send analytics, or is there a way to ensure that it's only the IDE plugins we own that can do so? If the former, I'd like to discuss this more offline. If going through DTD is the plan, then we might want to consider what the DTD message will look like at the same time in order to make sure everything works together. I'd still get rid of |
Companion to dart-lang/tools#2229 that noodles a bit on an analytics reporting API. If this looks good, I'll follow up with documentation and fill in some more implementation for the remaining actions (see: #8598). Implementing a `UnifiedAnalyticsReporter` that connects with the `UnifiedAnalytics` implementation may feed changes back to this too. (I'll schedule some time to chat through that since it'd probably be best done synchronously.) --- - [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR. <details> <summary>Contribution guidelines:</summary><br> - See our [contributor guide]([https://github.com/dart-lang/sdk/blob/main/CONTRIBUTING.md](https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Dart contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Java and Kotlin contributions should strive to follow Java and Kotlin best practices ([discussion](#8098)). </details>
Noodling on a retargetable
idePluginEvent.Example uses might look like:
Flutter App Actions (Reload, Restart, ReloadAll, RestartAll)
Flutter SDK Actions (Attach Debugger, Clean, Doctor, etc.)
Feedback and input most welcome!
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.