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

[native_assets_cli] Clarify meaning of targetAndroidNdkApi in docs #151

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

blaugold
Copy link
Contributor

@blaugold blaugold commented Oct 4, 2023

No description provided.

Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Thanks @blaugold !

@@ -68,6 +68,11 @@ class BuildConfig {

/// When compiling for Android, the API version to target.
///
/// This property is not to be confused with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe invert the sentence, so that we have what it is first. That is more important. Maybe even leave the what it is not out?

Something along the lines of:

  /// When compiling for Android, the minimum Android SDK API version to that the
  /// compiled code will be compatible with.
  ///
  /// Required when [targetOs] equals [OS.android].
  ///
  /// Not available in [dryRun].
  ///
  /// For more information about the Android API version, refer to
  /// [`minSdkVersion`](https://developer.android.com/ndk/guides/sdk-versions#minsdkversion)
  /// in the Android documentation.
  int? get targetAndroidNdkApi {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's much better. 😄

@coveralls
Copy link

coveralls commented Oct 4, 2023

Coverage Status

coverage: 98.177%. remained the same when pulling 7f3abb2 on blaugold:ndk-target-api-docs into 7aaa025 on dart-lang:main.

@auto-submit
Copy link

auto-submit bot commented Oct 4, 2023

auto label is removed for dart-lang/native/151, due to This PR has not met approval requirements for merging. You are not a member of dart-team and need 1 more review(s) in order to merge this PR.

  • Merge guidelines: You need at least one approved review if you are already part of flutter-hackers or two member reviews if you are not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@dcharkes dcharkes merged commit fd21f5b into dart-lang:main Oct 5, 2023
17 checks passed
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.

3 participants