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.

Sign up for GitHub

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

Jump to bottom

[pigeon] Adds SwiftFunction annotation #2304

Merged
merged 37 commits into from
Jan 26, 2023
Merged

[pigeon] Adds SwiftFunction annotation #2304

merged 37 commits into from
Jan 26, 2023

Conversation

ailtonvivaz
Copy link
Contributor

@ailtonvivaz ailtonvivaz commented Jul 3, 2022 •
edited by stuartmorgan
Loading

This PR adds a new annotation to Swift Generator to control the naming of a function like there is for ObjC (the annotation @ObjCSelector) as requested in flutter/flutter#105932

Fixes flutter/flutter#105932

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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • 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.

@stuartmorgan stuartmorgan requested review from gaaclarke and stuartmorgan and removed request for gaaclarke July 6, 2022 19:33
gaaclarke
gaaclarke requested changes Jul 21, 2022
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

The code and the tests looks good to me. The design for SwiftFunction looks good to me too. The docstrings just need a bit of cleanup. Awesome work.

packages/pigeon/lib/pigeon_lib.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
gaaclarke
gaaclarke approved these changes Jul 27, 2022
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm thanks!

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 27, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 27, 2022

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 27, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 27, 2022

Validations Fail.

stuartmorgan
stuartmorgan requested changes Aug 30, 2022
View reviewed changes
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.

The analyzer warnings are because the analysis settings for the repository changed recently, sorry for the churn.

packages/pigeon/lib/ast.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated
/// ie the name of function e the strings between the semicolons.
/// Example:
/// _swiftMethod('@SwiftFunction('add(_:with:)') void add(int x, int y)')
/// will return 'void add(int _ x, int with y)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remap the Dart APIs to a hybrid string that will never be used (and has no meaning in either language), only to re-extract the components later, instead of doing the mapping at the point of constructing the Swift code (as is done in the ObjC generator)?

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 followed this approach to minimize the changes. But, if this is not the better way, I could change to be similar with ObjC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having this intermediate format will likely make it harder to understand what's happening, and there's more potential for strange side effects (e.g., some new code trying to use the methods, and it not being obvious why they don't match the original source), so I'm concerned about the maintenance impact. I would much rather this transformation happen at the usage point even if it's more code in the short term.

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, I agree. I'll work on this update.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ailtonvivaz What is the status of this update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. Is this approach better?

stuartmorgan
stuartmorgan requested changes Sep 19, 2022
View reviewed changes
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.

Looks like I forgot to set the review state when I replied last time; updating the state to match the open comment above about moving when the name modification happens.

@ailtonvivaz
Merge branch 'main' into swift-function-annotation
d754a58 
# Conflicts:
#	packages/pigeon/CHANGELOG.md
#	packages/pigeon/lib/ast.dart
#	packages/pigeon/test/swift_generator_test.dart
@tarrinneal
Copy link
Contributor

It appears you've broken some swift code/tests.

Also, make sure to run the formatter cl tool with

dart pub global run flutter_plugin_tools format --packages pigeon

to clean up the generated files before uploading them.

@tarrinneal
Copy link
Contributor

Sorry, I was wrong about the breaking change. I reverted it back to 6.0.4.

stuartmorgan
stuartmorgan requested changes Jan 18, 2023
View reviewed changes
packages/pigeon/CHANGELOG.md Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/swift_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/pigeons/core_tests.dart Outdated Show resolved Hide resolved
@tarrinneal
Copy link
Contributor

tarrinneal commented Jan 18, 2023 •
edited
Loading

I think if you can fix these last few nits, and merge with main again, we should be able to land this shortly :)

Thank you for the time and effort you put into this pr!

@ailtonvivaz
Merge branch 'main' into swift-function-annotation
e734c28 
# Conflicts:
#	packages/pigeon/CHANGELOG.md
#	packages/pigeon/lib/generator_tools.dart
#	packages/pigeon/mock_handler_tester/test/message.dart
#	packages/pigeon/mock_handler_tester/test/test.dart
#	packages/pigeon/pigeons/core_tests.dart
#	packages/pigeon/platform_tests/alternate_language_test_plugin/android/src/main/java/com/example/alternate_language_test_plugin/CoreTests.java
#	packages/pigeon/platform_tests/alternate_language_test_plugin/ios/Classes/CoreTests.gen.h
#	packages/pigeon/platform_tests/alternate_language_test_plugin/ios/Classes/CoreTests.gen.m
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/core_tests.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/flutter_unittests.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/multiple_arity.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/non_null_fields.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/null_fields.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/nullable_returns.gen.dart
#	packages/pigeon/platform_tests/flutter_null_safe_unit_tests/lib/primitive.gen.dart
#	packages/pigeon/platform_tests/shared_test_plugin_code/lib/src/generated/core_tests.gen.dart
#	packages/pigeon/platform_tests/test_plugin/android/src/main/kotlin/com/example/test_plugin/CoreTests.gen.kt
#	packages/pigeon/platform_tests/test_plugin/ios/Classes/CoreTests.gen.swift
#	packages/pigeon/platform_tests/test_plugin/ios/Classes/TestPlugin.swift
#	packages/pigeon/platform_tests/test_plugin/macos/Classes/CoreTests.gen.swift
#	packages/pigeon/platform_tests/test_plugin/macos/Classes/TestPlugin.swift
#	packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.cpp
#	packages/pigeon/platform_tests/test_plugin/windows/pigeon/core_tests.gen.h
#	packages/pigeon/pubspec.yaml
@ailtonvivaz
Improve doc comments
0039165
@ailtonvivaz
Fix tests
651f9fa
tarrinneal
tarrinneal approved these changes Jan 25, 2023
View reviewed changes
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.

Good work!

stuartmorgan
stuartmorgan approved these changes Jan 25, 2023
View reviewed changes
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 with one comment nit. Thanks!

packages/pigeon/lib/pigeon_lib.dart Outdated Show resolved Hide resolved
@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2023
@auto-submit auto-submit bot merged commit 2d05534 into flutter:main Jan 26, 2023
sybrands-place pushed a commit to sybrands-place/packages that referenced this pull request Jan 30, 2023
Merge branch 'main' into null_status_retriable
577bbb1 
* main: (479 commits)
  removes raw ArrayLists (flutter#3101)
  Roll Flutter from c9affdb to 27f8ebd (15 revisions) (flutter#3098)
  [ci] Fix the new LUCI iOS build-all tasks (flutter#3099)
  [pigeon] [ObjC] Removes unused GetNullableObject function (flutter#3100)
  [pigeon] Swift host error handling (flutter#3084)
  Roll Flutter from a815ee6 to c9affdb (23 revisions) (flutter#3093)
  [ci] Enable min SDK version checks (flutter#3095)
  [pigeon] Fix C++ config handling (flutter#3094)
  [ci] Add LUCI version of iOS build-all (flutter#3096)
  [pigeon] Adds SwiftFunction annotation (flutter#2304)
  [flutter_adaptive_scaffold] Change `selectedIndex` on `standardNavigationRail` to allow null value. (flutter#3088)
  [pigeon] requires analyzer 5.2.0 (flutter#3090)
  Roll Flutter from c35efda to a815ee6 (22 revisions) (flutter#3089)
  [ci] Update legacy Flutter version tests (flutter#3087)
  Roll Flutter (stable) from 135454af3247 to b06b8b271095 (2551 revisions) (flutter#3086)
  [flutter_adaptive_scaffold] Fix leading and trailing Navigation Rail Widgets (flutter#3080)
  Roll Flutter from bd7bee0 to c35efda (24 revisions) (flutter#3085)
  [pigeon] Minor C++ output adjustments (flutter#3083)
  [pigeon] Updates writeScoped and addScoped to disallow symbol-less use. (flutter#3081)
  Roll Flutter from f33e8d3 to bd7bee0 (5 revisions) (flutter#3082)
  ...
Maatteogekko pushed a commit to Maatteogekko/packages that referenced this pull request Feb 4, 2023
@ailtonvivaz @gaaclarke
@tarrinneal @Maatteogekko
[pigeon] Adds SwiftFunction annotation (flutter#2304)
4a39070 
* Add SwiftFunction annotation

* Bump version to 3.2.4

* Remove unused imports

* Improve methods map

* Remove unnecessary print

* Force cast match of SwiftFunction

* Update packages/pigeon/lib/pigeon_lib.dart

Co-authored-by: gaaclarke <[email protected]>

* Improve documentation of function to parse method with SwiftFunction

* Fix some dartdocs

* gen

* analyze

* Improve SwiftFunction application

* Add type annotation

* format

* Run format

* Update macos Swift tests

* Bump version to 7.0.0

* revert version change

* Improve some code of SwiftGenerator

* Bump version to 6.1.0

* Improve echo functions for Swift

* Match order of parameters

* Documents _SwiftFunctionComponents.fromMethod and _SwiftFunctionArgument

* Improve doc comments

* Fix tests

* Fix SwiftFunction documentation

Co-authored-by: gaaclarke <[email protected]>
Co-authored-by: tarrinneal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarrinneal tarrinneal tarrinneal approved these changes

@gaaclarke gaaclarke gaaclarke approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[pigeon] New annotation for Swift function naming
4 participants
@ailtonvivaz @stuartmorgan @tarrinneal @gaaclarke