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

[local_auth_darwin] - Converts implementation to Swift #7586

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

Mairramer
Copy link
Contributor

@Mairramer Mairramer commented Sep 5, 2024

Part of flutter/flutter#119015
Closes flutter/flutter#119104

Pre-launch Checklist

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

@Mairramer Mairramer closed this Oct 19, 2024
@Mairramer Mairramer reopened this Nov 6, 2024
# 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
@stuartmorgan
Copy link
Contributor

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.

@Mairramer
Copy link
Contributor Author

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.

@stuartmorgan
Copy link
Contributor

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.

As I said in my previous comment:

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.

This PR is not the place to make structural changes that are based on your personal preferences.

@Mairramer
Copy link
Contributor Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[local_auth] iOS Swift migration
3 participants