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

[pigeon] Allows swift generator to skip error class generation #7849

Merged
merged 11 commits into from
Oct 23, 2024

Conversation

wamynobe
Copy link
Contributor

@wamynobe wamynobe commented Oct 11, 2024

Since the Kotlin generator allows skipping error class generation, it makes sense for the Swift generator to have the same option.

Related to:
#6183
flutter/flutter#142099

Pre-launch Checklist

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

Copy link

google-cla bot commented Oct 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@wamynobe wamynobe force-pushed the swift-error-sharing branch from 05e20e9 to 5faea22 Compare October 12, 2024 14:53
@wamynobe wamynobe force-pushed the swift-error-sharing branch from 19fd315 to 79a54b0 Compare October 12, 2024 15:01
@wamynobe
Copy link
Contributor Author

Hi @tarrinneal,
In your previous PR #6183 , there are a few parts that I don't really understand (mostly about how to generate test files), for example the code kotlinIncludeErrorClass: input != 'core_tests', in packages/pigeon/tool/shared/generation.dart. Could you please guide me through these test cases?

@wamynobe wamynobe marked this pull request as ready for review October 17, 2024 07:06
@wamynobe wamynobe requested a review from tarrinneal as a code owner October 17, 2024 07:06
@wamynobe
Copy link
Contributor Author

looks like the test cases using the default error class name in Swift have failed.

@wamynobe
Copy link
Contributor Author

How can I run those custom_package_tests test cases on my local machine?

@tarrinneal
Copy link
Contributor

if you navigate to the pigeon directory you can run dart run tool/test.dart -f -t ios_swift_integration_tests

@tarrinneal
Copy link
Contributor

tarrinneal commented Oct 17, 2024

This code

    final bool kotlinErrorClassGenerationTestFiles =
        input == 'core_tests' || input == 'background_platform_channels';

    final String kotlinErrorName = kotlinErrorClassGenerationTestFiles
        ? 'FlutterError'
        : '${pascalCaseName}Error';

and the corresponding usage of kotlinErrorName ensures that there are two files with the same error class name. You'll need to do that for swift as well.

@wamynobe
Copy link
Contributor Author

wamynobe commented Oct 18, 2024

if you navigate to the pigeon directory you can run dart run tool/test.dart -f -t ios_swift_integration_tests

Thanks @tarrinneal I am now able to run those test cases.

and the corresponding usage of kotlinErrorName ensures that there are two files with the same error class name. You'll need to do that for swift as well.

I saw swiftErrorUseDefaultErrorName which has the same purpose as kotlinErrorClassGenerationTestFiles. Updated!

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together! (looks like I forgot to submit my approval last week, sorry)

@tarrinneal tarrinneal added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Oct 23, 2024
@tarrinneal
Copy link
Contributor

@stuartmorgan for 2nd review

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 23, 2024
@auto-submit auto-submit bot merged commit ff82995 into flutter:main Oct 23, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 24, 2024
flutter/packages@5e03bb1...a556f0f

2024-10-24 [email protected] [camera] Manual roll and skip failing tests (flutter/packages#7891)
2024-10-23 49699333+dependabot[bot]@users.noreply.github.com [image_picker]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.0 in /packages/image_picker/image_picker_android/android (flutter/packages#7898)
2024-10-23 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.0 in /packages/url_launcher/url_launcher_android/android (flutter/packages#7906)
2024-10-23 [email protected] [pigeon] Allows swift generator to skip error class generation (flutter/packages#7849)
2024-10-23 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7903)
2024-10-23 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.0 in /packages/file_selector/file_selector_android/android (flutter/packages#7900)
2024-10-23 [email protected] [video_player_android] Give `can pause` integration test range to test for (flutter/packages#7913)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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 p: pigeon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants