-
Couldn't load subscription status.
- Fork 76
Add more data to Event.analysisStatistics #2219
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 adds several new fields to the Event.analysisStatistics event to provide more detailed analytics data. The changes are well-documented and tested. My feedback focuses on improving code readability and maintainability by sorting parameters and map keys alphabetically, which aligns with the repository's style guide. I've also pointed out a minor formatting issue in a documentation comment.
| /// * [immediateFileCountPercentiles] - json encoded percentile values for the | ||
| /// number of files in the immediate workspace. |
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 indentation for the wrapped line in this comment is incorrect. It should be aligned with the start of the text on the previous line to improve readability, following standard Dart documentation style.
| /// * [immediateFileCountPercentiles] - json encoded percentile values for the | |
| /// number of files in the immediate workspace. | |
| /// * [immediateFileCountPercentiles] - json encoded percentile values for the | |
| /// number of files in the immediate workspace. |
| required String workingDuration, | ||
| required bool withFineDependencies, | ||
| required int changedFileEventCount, | ||
| required int removedFileEventCount, | ||
| required int changedFileUniqueCount, | ||
| required int removedFileUniqueCount, | ||
| required String immediateFileCountPercentiles, | ||
| required String immediateFileLineCountPercentiles, | ||
| required String transitiveFileCountPercentiles, | ||
| required String transitiveFileLineCountPercentiles, | ||
| required int produceErrorsPotentialFileCount, | ||
| required int produceErrorsPotentialFileLineCount, | ||
| required int produceErrorsActualFileCount, | ||
| required int produceErrorsActualFileLineCount, | ||
| required int produceErrorsDurationMs, | ||
| required int produceErrorsElementsDurationMs, |
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 named parameters in this constructor are not in alphabetical order. Sorting them alphabetically improves readability and makes it easier to find specific parameters. This aligns with the style guide's emphasis on readability.1
| required String workingDuration, | |
| required bool withFineDependencies, | |
| required int changedFileEventCount, | |
| required int removedFileEventCount, | |
| required int changedFileUniqueCount, | |
| required int removedFileUniqueCount, | |
| required String immediateFileCountPercentiles, | |
| required String immediateFileLineCountPercentiles, | |
| required String transitiveFileCountPercentiles, | |
| required String transitiveFileLineCountPercentiles, | |
| required int produceErrorsPotentialFileCount, | |
| required int produceErrorsPotentialFileLineCount, | |
| required int produceErrorsActualFileCount, | |
| required int produceErrorsActualFileLineCount, | |
| required int produceErrorsDurationMs, | |
| required int produceErrorsElementsDurationMs, | |
| required int changedFileEventCount, | |
| required int changedFileUniqueCount, | |
| required String immediateFileCountPercentiles, | |
| required String immediateFileLineCountPercentiles, | |
| required int produceErrorsActualFileCount, | |
| required int produceErrorsActualFileLineCount, | |
| required int produceErrorsDurationMs, | |
| required int produceErrorsElementsDurationMs, | |
| required int produceErrorsPotentialFileCount, | |
| required int produceErrorsPotentialFileLineCount, | |
| required int removedFileEventCount, | |
| required int removedFileUniqueCount, | |
| required String transitiveFileCountPercentiles, | |
| required String transitiveFileLineCountPercentiles, | |
| required bool withFineDependencies, | |
| required String workingDuration, |
Style Guide References
Footnotes
-
Code should be optimized for readability, as it is read more often than it is written. Sorting parameters alphabetically improves readability. ↩
| 'workingDuration': workingDuration, | ||
| 'withFineDependencies': withFineDependencies, | ||
| 'changedFileUniqueCount': changedFileUniqueCount, | ||
| 'removedFileUniqueCount': removedFileUniqueCount, | ||
| 'changedFileEventCount': changedFileEventCount, | ||
| 'removedFileEventCount': removedFileEventCount, | ||
| 'immediateFileCountPercentiles': immediateFileCountPercentiles, | ||
| 'immediateFileLineCountPercentiles': | ||
| immediateFileLineCountPercentiles, | ||
| 'transitiveFileCountPercentiles': transitiveFileCountPercentiles, | ||
| 'transitiveFileLineCountPercentiles': | ||
| transitiveFileLineCountPercentiles, | ||
| 'produceErrorsPotentialFileCount': produceErrorsPotentialFileCount, | ||
| 'produceErrorsPotentialFileLineCount': | ||
| produceErrorsPotentialFileLineCount, | ||
| 'produceErrorsActualFileCount': produceErrorsActualFileCount, | ||
| 'produceErrorsActualFileLineCount': | ||
| produceErrorsActualFileLineCount, | ||
| 'produceErrorsDurationMs': produceErrorsDurationMs, | ||
| 'produceErrorsElementsDurationMs': produceErrorsElementsDurationMs, |
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 keys in the eventData map are not in alphabetical order. Sorting them makes the code more readable and consistent with the suggested sorted constructor parameters. This aligns with the style guide's goal of optimizing for readability.1
| 'workingDuration': workingDuration, | |
| 'withFineDependencies': withFineDependencies, | |
| 'changedFileUniqueCount': changedFileUniqueCount, | |
| 'removedFileUniqueCount': removedFileUniqueCount, | |
| 'changedFileEventCount': changedFileEventCount, | |
| 'removedFileEventCount': removedFileEventCount, | |
| 'immediateFileCountPercentiles': immediateFileCountPercentiles, | |
| 'immediateFileLineCountPercentiles': | |
| immediateFileLineCountPercentiles, | |
| 'transitiveFileCountPercentiles': transitiveFileCountPercentiles, | |
| 'transitiveFileLineCountPercentiles': | |
| transitiveFileLineCountPercentiles, | |
| 'produceErrorsPotentialFileCount': produceErrorsPotentialFileCount, | |
| 'produceErrorsPotentialFileLineCount': | |
| produceErrorsPotentialFileLineCount, | |
| 'produceErrorsActualFileCount': produceErrorsActualFileCount, | |
| 'produceErrorsActualFileLineCount': | |
| produceErrorsActualFileLineCount, | |
| 'produceErrorsDurationMs': produceErrorsDurationMs, | |
| 'produceErrorsElementsDurationMs': produceErrorsElementsDurationMs, | |
| 'changedFileEventCount': changedFileEventCount, | |
| 'changedFileUniqueCount': changedFileUniqueCount, | |
| 'immediateFileCountPercentiles': immediateFileCountPercentiles, | |
| 'immediateFileLineCountPercentiles': | |
| immediateFileLineCountPercentiles, | |
| 'produceErrorsActualFileCount': produceErrorsActualFileCount, | |
| 'produceErrorsActualFileLineCount': | |
| produceErrorsActualFileLineCount, | |
| 'produceErrorsDurationMs': produceErrorsDurationMs, | |
| 'produceErrorsElementsDurationMs': produceErrorsElementsDurationMs, | |
| 'produceErrorsPotentialFileCount': produceErrorsPotentialFileCount, | |
| 'produceErrorsPotentialFileLineCount': | |
| produceErrorsPotentialFileLineCount, | |
| 'removedFileEventCount': removedFileEventCount, | |
| 'removedFileUniqueCount': removedFileUniqueCount, | |
| 'transitiveFileCountPercentiles': transitiveFileCountPercentiles, | |
| 'transitiveFileLineCountPercentiles': | |
| transitiveFileLineCountPercentiles, | |
| 'withFineDependencies': withFineDependencies, | |
| 'workingDuration': workingDuration, |
Style Guide References
Footnotes
-
Code should be optimized for readability. Sorting map keys alphabetically improves readability, especially for large maps. ↩
| Event generateEvent() => Event.analysisStatistics( | ||
| workingDuration: 'workingDuration', | ||
| withFineDependencies: false, | ||
| changedFileEventCount: 1, | ||
| removedFileEventCount: 2, | ||
| changedFileUniqueCount: 3, | ||
| removedFileUniqueCount: 4, | ||
| immediateFileCountPercentiles: 'immediateFileCountPercentiles', | ||
| immediateFileLineCountPercentiles: | ||
| 'immediateFileLineCountPercentiles', | ||
| transitiveFileCountPercentiles: 'transitiveFileCountPercentiles', | ||
| transitiveFileLineCountPercentiles: | ||
| 'transitiveFileLineCountPercentiles', | ||
| produceErrorsPotentialFileCount: 10, | ||
| produceErrorsPotentialFileLineCount: 11, | ||
| produceErrorsActualFileCount: 12, | ||
| produceErrorsActualFileLineCount: 13, | ||
| produceErrorsDurationMs: 14, | ||
| produceErrorsElementsDurationMs: 15, | ||
| ); |
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.
For consistency with the suggested changes in event.dart and to improve readability, the parameters in this Event.analysisStatistics call should be sorted alphabetically. This aligns with the style guide's emphasis on readability.1
| Event generateEvent() => Event.analysisStatistics( | |
| workingDuration: 'workingDuration', | |
| withFineDependencies: false, | |
| changedFileEventCount: 1, | |
| removedFileEventCount: 2, | |
| changedFileUniqueCount: 3, | |
| removedFileUniqueCount: 4, | |
| immediateFileCountPercentiles: 'immediateFileCountPercentiles', | |
| immediateFileLineCountPercentiles: | |
| 'immediateFileLineCountPercentiles', | |
| transitiveFileCountPercentiles: 'transitiveFileCountPercentiles', | |
| transitiveFileLineCountPercentiles: | |
| 'transitiveFileLineCountPercentiles', | |
| produceErrorsPotentialFileCount: 10, | |
| produceErrorsPotentialFileLineCount: 11, | |
| produceErrorsActualFileCount: 12, | |
| produceErrorsActualFileLineCount: 13, | |
| produceErrorsDurationMs: 14, | |
| produceErrorsElementsDurationMs: 15, | |
| ); | |
| Event generateEvent() => Event.analysisStatistics( | |
| changedFileEventCount: 1, | |
| changedFileUniqueCount: 3, | |
| immediateFileCountPercentiles: 'immediateFileCountPercentiles', | |
| immediateFileLineCountPercentiles: | |
| 'immediateFileLineCountPercentiles', | |
| produceErrorsActualFileCount: 12, | |
| produceErrorsActualFileLineCount: 13, | |
| produceErrorsDurationMs: 14, | |
| produceErrorsElementsDurationMs: 15, | |
| produceErrorsPotentialFileCount: 10, | |
| produceErrorsPotentialFileLineCount: 11, | |
| removedFileEventCount: 2, | |
| removedFileUniqueCount: 4, | |
| transitiveFileCountPercentiles: 'transitiveFileCountPercentiles', | |
| transitiveFileLineCountPercentiles: | |
| 'transitiveFileLineCountPercentiles', | |
| withFineDependencies: false, | |
| workingDuration: 'workingDuration', | |
| ); |
Style Guide References
Footnotes
-
Code should be optimized for readability. Sorting parameters alphabetically in test data also improves readability and maintainability. ↩
| expect(constructedEvent.eventData['workingDuration'], 'workingDuration'); | ||
| expect(constructedEvent.eventData.length, 1); | ||
| expect(constructedEvent.eventData['withFineDependencies'], false); | ||
| expect(constructedEvent.eventData['changedFileEventCount'], 1); | ||
| expect(constructedEvent.eventData['removedFileEventCount'], 2); | ||
| expect(constructedEvent.eventData['changedFileUniqueCount'], 3); | ||
| expect(constructedEvent.eventData['removedFileUniqueCount'], 4); | ||
| expect(constructedEvent.eventData['immediateFileCountPercentiles'], | ||
| 'immediateFileCountPercentiles'); | ||
| expect(constructedEvent.eventData['immediateFileLineCountPercentiles'], | ||
| 'immediateFileLineCountPercentiles'); | ||
| expect(constructedEvent.eventData['transitiveFileCountPercentiles'], | ||
| 'transitiveFileCountPercentiles'); | ||
| expect(constructedEvent.eventData['transitiveFileLineCountPercentiles'], | ||
| 'transitiveFileLineCountPercentiles'); | ||
| expect(constructedEvent.eventData['produceErrorsPotentialFileCount'], 10); | ||
| expect( | ||
| constructedEvent.eventData['produceErrorsPotentialFileLineCount'], 11); | ||
| expect(constructedEvent.eventData['produceErrorsActualFileCount'], 12); | ||
| expect(constructedEvent.eventData['produceErrorsActualFileLineCount'], 13); | ||
| expect(constructedEvent.eventData['produceErrorsDurationMs'], 14); | ||
| expect(constructedEvent.eventData['produceErrorsElementsDurationMs'], 15); |
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.
To improve readability and consistency, the expect calls checking the eventData properties should be sorted alphabetically by key. This makes the test easier to read and verify.1
expect(constructedEvent.eventData['changedFileEventCount'], 1);
expect(constructedEvent.eventData['changedFileUniqueCount'], 3);
expect(constructedEvent.eventData['immediateFileCountPercentiles'],
'immediateFileCountPercentiles');
expect(constructedEvent.eventData['immediateFileLineCountPercentiles'],
'immediateFileLineCountPercentiles');
expect(constructedEvent.eventData['produceErrorsActualFileCount'], 12);
expect(constructedEvent.eventData['produceErrorsActualFileLineCount'], 13);
expect(constructedEvent.eventData['produceErrorsDurationMs'], 14);
expect(constructedEvent.eventData['produceErrorsElementsDurationMs'], 15);
expect(constructedEvent.eventData['produceErrorsPotentialFileCount'], 10);
expect(
constructedEvent.eventData['produceErrorsPotentialFileLineCount'], 11);
expect(constructedEvent.eventData['removedFileEventCount'], 2);
expect(constructedEvent.eventData['removedFileUniqueCount'], 4);
expect(constructedEvent.eventData['transitiveFileCountPercentiles'],
'transitiveFileCountPercentiles');
expect(constructedEvent.eventData['transitiveFileLineCountPercentiles'],
'transitiveFileLineCountPercentiles');
expect(constructedEvent.eventData['withFineDependencies'], false);
expect(constructedEvent.eventData['workingDuration'], 'workingDuration');Style Guide References
Footnotes
-
Code should be optimized for readability. Sorting assertions in tests makes them easier to read and verify. ↩
PR Health
Breaking changes
|
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| unified_analytics | Non-Breaking | 8.0.5 | 8.0.7 | 8.1.0 Got "8.0.7" expected >= "8.1.0" (non-breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
Coverage ⚠️
| File | Coverage |
|---|---|
| pkgs/unified_analytics/lib/src/constants.dart | 💔 Not covered |
| pkgs/unified_analytics/lib/src/event.dart | 💚 98 % |
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.
| /// | ||
| /// The background analysis group includes: | ||
| /// * [produceErrorsPotentialFileCount] - the total number of files that could | ||
| /// potentially produce errors during background analysis. |
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.
"that could potentially produce errors"
Would it be more accurate to say "for which the reported diagnostics could potentially change"?
For https://dart-review.googlesource.com/c/sdk/+/453864