-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[local_auth_darwin] - Converts implementation to Swift #7586
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/local_auth/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/include/local_auth_darwin/messages.g.h # packages/local_auth/local_auth_darwin/darwin/local_auth_darwin/Sources/local_auth_darwin/messages.g.m
As a high-level comment on my specific comments above: as discussed earlier, the goal of a Swift conversion PR should be to convert the existing logic to Swift as directly as possible, modulo any language-specific idiomatic differences. A conversion is already a relatively high-risk change due to the magnitude of code being changed; intentionally mixing in refactors, subjective changes to implementation approach, etc. is highly undesirable in such a PR because it increases the risk even futher. Any delta between the Obj-C logic and the Swift logic in this PR should be clearly and directly attributable to a language-specific reason. If you have subjective disagreements about how the code is currently structured, please feel free to either make prequel PRs or follow-up PRs that make those changes, so they can be discussed, reviewed, and tested in isolation, rather than as part of a complete rewrite of the code. |
I am reworking some parts you mentioned again and trying to keep the test mocks as close as possible to how they were before. There are many things I would like to remove, like the factory. While I understand the reasoning behind its use, it's not something I personally prefer. I'm working on this more slowly because I have limited time to dedicate to it. |
As I said in my previous comment:
This PR is not the place to make structural changes that are based on your personal preferences. |
@stuartmorgan I made some adjustments and I'm closer to the Objective-C implementation. There are still a few things left to be done, and if you think it's appropriate, I can mark this PR as a draft and continue working on the remaining details |
Part of flutter/flutter#119015
Closes flutter/flutter#119104
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.