Skip to content

Conversation

@eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Nov 9, 2023

Related to tracker issue:

This event was only called from one file (flutter_command.dart). With the previous implementation, we actually sent 2 events, one for the result of the commandPath and another containing the maxRss value from ProcessInfo.

I have consolidated this down to just one event and used a function to safely get the maxRss value, or return null when if there was an error getting that integer value

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 9, 2023
Comment on lines 1605 to 1610
FlutterCommandResult commandResult,
DateTime startTime,
DateTime endTime,
Analytics analytics,
ProcessInfo processInfo,
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christopherfujino, is it enough to inject it at this level? Otherwise, I would need to inject these 2 parameters in each of the 70+ FlutterCommand implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or I could possibly create a getter in the abstract class FlutterCommand to fetch these two classes from globals so that each implementation of FlutterCommand has access to it

abstract class FlutterCommand extends Command<void> {
  
  Analytics get analytics => globals.analytics;
  ProcessInfo get processInfo => globals.processInfo;
  
  //.....
}

Copy link
Contributor

@christopherfujino christopherfujino Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know what, I like that approach (creating getters on FlutterCommand). Then later on, we can set up a method to lazily initialize them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, that will also make it easy for any other command to directly inject the analytics instance too

Copy link
Contributor

@christopherfujino christopherfujino Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for posterity, when I say "a method to lazily initialize them", I mean a process, not literally a class method.

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eliasyishak eliasyishak added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2023
@auto-submit auto-submit bot merged commit 7a278ae into flutter:master Nov 10, 2023
@eliasyishak eliasyishak deleted the ga4-migrate-command-result branch November 10, 2023 20:22
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 13, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 14, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 14, 2023
Roll Flutter from f662150 to e8c2bb1 (66 revisions)

flutter/flutter@f662150...e8c2bb1

2023-11-14 [email protected] Reland "Update `framework_test.dart` to remove `ButtonBar` usage and remove references from other clases (#137550) (flutter/flutter#137753)
2023-11-14 [email protected] Consume flutter.js from the engine artifacts. (flutter/flutter#137113)
2023-11-14 [email protected] Roll Flutter Engine from 995ebcefb0d4 to 1b3fd80812c3 (1 revision) (flutter/flutter#138415)
2023-11-14 [email protected] Roll Packages from 17bd92e to 428ba3e (2 revisions) (flutter/flutter#138412)
2023-11-14 [email protected] Roll Flutter Engine from 712094481217 to 995ebcefb0d4 (1 revision) (flutter/flutter#138409)
2023-11-14 [email protected] Roll Flutter Engine from 2c54b5f70c94 to 712094481217 (1 revision) (flutter/flutter#138401)
2023-11-14 [email protected] Roll Flutter Engine from d100e8912eb0 to 2c54b5f70c94 (4 revisions) (flutter/flutter#138399)
2023-11-14 [email protected] Roll Flutter Engine from 74a9de45f128 to d100e8912eb0 (3 revisions) (flutter/flutter#138383)
2023-11-14 [email protected] Roll Flutter Engine from 77b952f3add4 to 74a9de45f128 (3 revisions) (flutter/flutter#138382)
2023-11-14 [email protected] Fixing typo (flutter/flutter#138253)
2023-11-14 [email protected] Roll Flutter Engine from 046ec85dffc6 to 77b952f3add4 (1 revision) (flutter/flutter#138377)
2023-11-13 [email protected] Roll Flutter Engine from db6da000a17e to 046ec85dffc6 (5 revisions) (flutter/flutter#138375)
2023-11-13 [email protected] Finally remove analysis_options_user.yaml (flutter/flutter#138261)
2023-11-13 [email protected] Add a DevTools section to CONTRIBUTING.md (flutter/flutter#137193)
2023-11-13 [email protected] Update DraggableScrollableSheet docs to reflect API change (flutter/flutter#136471)
2023-11-13 [email protected] Clean up synonyms, key code generation. (flutter/flutter#138192)
2023-11-13 [email protected] Roll Flutter Engine from 74ba6c17a488 to db6da000a17e (2 revisions) (flutter/flutter#138364)
2023-11-13 [email protected] Roll Flutter Engine from 5d62f1a2392a to 74ba6c17a488 (1 revision) (flutter/flutter#138362)
2023-11-13 [email protected] Roll Flutter Engine from 7de793d2bb68 to 5d62f1a2392a (1 revision) (flutter/flutter#138358)
2023-11-13 [email protected] Upgrade leak tracker. (flutter/flutter#138283)
2023-11-13 [email protected] Roll Flutter Engine from fe11f3a46bac to 7de793d2bb68 (2 revisions) (flutter/flutter#138353)
2023-11-13 [email protected] Roll Packages from a682189 to 17bd92e (2 revisions) (flutter/flutter#138347)
2023-11-13 [email protected] Roll Flutter Engine from a18ee3c7f57a to fe11f3a46bac (2 revisions) (flutter/flutter#138344)
2023-11-12 [email protected] Roll Flutter Engine from 828e4dbf6693 to a18ee3c7f57a (7 revisions) (flutter/flutter#138332)
2023-11-11 [email protected] Roll Flutter Engine from e2e07eab35ec to 828e4dbf6693 (1 revision) (flutter/flutter#138282)
2023-11-11 [email protected] Roll Flutter Engine from 1c29ce15c528 to e2e07eab35ec (1 revision) (flutter/flutter#138280)
2023-11-11 [email protected] Roll Flutter Engine from aa6753fdbb51 to 1c29ce15c528 (1 revision) (flutter/flutter#138277)
2023-11-11 [email protected] Roll Flutter Engine from 00db306f6f7b to aa6753fdbb51 (1 revision) (flutter/flutter#138269)
2023-11-11 [email protected] Roll Flutter Engine from 9d8a1125640d to 00db306f6f7b (7 revisions) (flutter/flutter#138266)
2023-11-10 [email protected] Only run tests on macOS 12 (flutter/flutter#138260)
2023-11-10 [email protected] Fixes vscode path installed via snap (flutter/flutter#136997)
2023-11-10 [email protected] Docs typo: comprised -> composed (flutter/flutter#137896)
2023-11-10 [email protected] Deprecates onWillAccept and onAccept callbacks in DragTarget. (flutter/flutter#133691)
2023-11-10 [email protected] [macOS] Suppress Xcode 15 createItemModels warning (flutter/flutter#138243)
2023-11-10 [email protected] Roll Flutter Engine from 275ddb296ec9 to 9d8a1125640d (1 revision) (flutter/flutter#138252)
2023-11-10 [email protected] GestureRecognizer should dispatch creation and disposal events. (flutter/flutter#138223)
2023-11-10 [email protected] Roll Flutter Engine from 5c2e16c5a95a to 275ddb296ec9 (1 revision) (flutter/flutter#138249)
2023-11-10 [email protected] Roll Packages from b69f54e to a682189 (3 revisions) (flutter/flutter#138239)
2023-11-10 [email protected] Roll Flutter Engine from e5b75177ac8e to 5c2e16c5a95a (1 revision) (flutter/flutter#138247)
2023-11-10 [email protected] `CommandResultEvent` migrated (flutter/flutter#138165)
2023-11-10 [email protected] Fix #128925 by properly setting the Android Event Source (flutter/flutter#138241)
2023-11-10 [email protected] Roll Flutter Engine from 77349dc8e27b to e5b75177ac8e (2 revisions) (flutter/flutter#138244)
2023-11-10 [email protected] Roll Flutter Engine from a5eab0d281fe to 77349dc8e27b (4 revisions) (flutter/flutter#138237)
2023-11-10 [email protected] Update analytics constructor to include `FLUTTER_HOST` (flutter/flutter#138107)
2023-11-10 [email protected] Prepare the analyze_once test for removal of analysis_options_user support (flutter/flutter#138229)
2023-11-10 [email protected] Roll Flutter Engine from b020893cba29 to a5eab0d281fe (5 revisions) (flutter/flutter#138231)
...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants