Skip to content
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 added for commandUsageValues #211

Merged

Conversation

eliasyishak
Copy link
Contributor

@eliasyishak eliasyishak commented Nov 30, 2023

Related to tracker issue:

This is a migration for the flutter Usage.command static method. When calling this method from FlutterCommand or a child class, it will fetch the necessary CustomDimensions for that command. There are a few child classes of FlutterCommand that override the usageValues getter to provide their own custom dimensions.

As a result, this means that parameters we need to send for each of those classes varies. So instead of creating a separate Event constructor for each of these child classes, we will instead have just one constructor that contains a superset of all possible custom dimension parameters. This makes the parameter count for this Event large (even larger than the maximum 25 parameters allowed). However, when sending this class from the flutter tool, we will ensure that the number of parameters used is going to be below the max (25) parameter count.

Snippet of what the Event below

  Event.commandUsageValues({
    required String workflow,
    required bool commandHasTerminal,

    // Assemble && build bundle implementation parameters
    String? buildBundleTargetPlatform,
    bool? buildBundleIsModule,

    // Build aar implementation parameters
    String? buildAarProjectType,
    String? buildAarTargetPlatform,

    // Build apk implementation parameters
    String? buildApkTargetPlatform,
    String? buildApkBuildMode,
    bool? buildApkSplitPerAbi,

    // Build app bundle implementation parameters
    String? buildAppBundleTargetPlatform,
    String? buildAppBundleBuildMode,

    // Create implementation parameters
    String? createProjectType,
    String? createAndroidLanguage,
    String? createIosLanguage,

    // Packages implementation parameters
    int? packagesNumberPlugins,
    bool? packagesProjectModule,
    String? packagesAndroidEmbeddingVersion,

    // Run implementation parameters
    bool? runIsEmulator,
    String? runTargetName,
    String? runTargetOsVersion,
    String? runModeName,
    bool? runProjectModule,
    String? runProjectHostLanguage,
    String? runAndroidEmbeddingVersion,
    bool? runEnableImpeller,
    String? runIOSInterfaceType,
    bool? runIsTest,
  })

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:unified_analytics 5.7.0 ready to publish unified_analytics-v5.7.0
package:graphs 2.3.2-wip WIP (no publish necessary)
package:extension_discovery 2.0.1-wip WIP (no publish necessary)
package:cli_config 0.1.2 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

/// The custom dimensions from each implementation are grouped by sharing
/// the same prefix. The [workflow] indicates which implementation is being
/// sent, for example, "create".
Event.commandUsageValues({
Copy link
Member

Choose a reason for hiding this comment

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

This is a non-obvious name to me (that doesn't mean it isn't the best, just that it's not obvious to my brain). What about just Event.command?

Or, do we need to namespace this by the flutter tool somehow? Even though I can see other tools wanting to track its sub-commands, these fields are specific to the flutter tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually had the same thought myself. I'm open to changing the constructor name though, i just tried to make it reflect what it was doing.

But I agree, as we start adding several more events, it's going to be a bit harder to know which event belongs to which dash tool so i created the issue below to at least add the tool it belongs to in the top line in the dartdoc

Copy link
Member

Choose a reason for hiding this comment

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

It's your call as to the name

Copy link
Member

@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 merged commit c4d515a into dart-lang:main Dec 5, 2023
6 checks passed
@eliasyishak eliasyishak deleted the add-command-usage-value-event branch December 5, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants