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

refactor: rewrite OpenAI service with Swift #473

Merged
merged 22 commits into from
Apr 2, 2024
Merged

refactor: rewrite OpenAI service with Swift #473

merged 22 commits into from
Apr 2, 2024

Conversation

tisfeng
Copy link
Owner

@tisfeng tisfeng commented Mar 26, 2024

close #291

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 26, 2024

There are no feature changes in this PR, just a rewrite of the OpenAI service using Swift and removing the old objc code.

Other changes:

  • Hardcoded API keys were put into plist files.
  • Adjusted the file structure to move all Swift code to Swift files.

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 26, 2024

There seems to be something wrong with CI, can anyone help take a look?

@phlpsong
Copy link
Collaborator

Screenshot 2024-03-26 at 22 08 59

Looks like a package resolution issue because of Package.resolved file is corrupted or malformed, I cannot build it in local machine either.

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 26, 2024

@phlpsong I didn't change the configuration of custom OpenAI, I don't know why its interface changed to be the same as OpenAI.

I debug found that it will call OpenAIService + ConfigurableService instead of CustomOpenAIService + ConfigurableService, can you help me to look at it?

image image

@phlpsong
Copy link
Collaborator

phlpsong commented Mar 27, 2024

I think the root cause is the change of inheritance of OpenAIService and CustomOpenAIService, currently, CustomOpenAIService inherits from OpenAIService and they both implement ConfigurableService, this may caused the method dispatch problem.

Maybe the simple approach is to create a base class like before, and make the subclass conform to ConfigurableService.

FYI: https://stackoverflow.com/questions/44703205/swift-protocol-extension-method-is-called-instead-of-method-implemented-in-subcl

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 27, 2024

@phlpsong ok, thank you for your answer, I will check it later.

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 28, 2024

I added a BaseOpenAIService, it looks fixed now.

@phlpsong
Copy link
Collaborator

There seems to be something wrong with CI, can anyone help take a look?

I just found out that Xcode 15.3 could fix this. Currently, CI uses 15.1.

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 30, 2024

The system images of CI and Xcode versions are usually a bit outdated. As long as they run without issues, we can temporarily keep them until adapting to the new major updates.

@phlpsong
Copy link
Collaborator

Sure, let's keep the current package configuration utils stable.

@tisfeng
Copy link
Owner Author

tisfeng commented Mar 30, 2024

Please review this PR 🥹

Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/BaseOpenAIService.swift Outdated Show resolved Hide resolved
Easydict/Swift/Service/OpenAI/OpenAIService.swift Outdated Show resolved Hide resolved
Easydict/objc/ViewController/Cell/EZTableTipsCell.m Outdated Show resolved Hide resolved
@tisfeng tisfeng requested a review from phlpsong April 1, 2024 02:50
@tisfeng
Copy link
Owner Author

tisfeng commented Apr 1, 2024

All fixed, please continue to review.

Jerry23011
Jerry23011 previously approved these changes Apr 1, 2024
@tisfeng tisfeng requested a review from phlpsong April 1, 2024 15:19
phlpsong
phlpsong previously approved these changes Apr 1, 2024
Copy link
Collaborator

@phlpsong phlpsong left a comment

Choose a reason for hiding this comment

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

LGTM

phlpsong
phlpsong previously approved these changes Apr 2, 2024
@tisfeng tisfeng requested a review from Jerry23011 April 2, 2024 02:35
@tisfeng tisfeng merged commit 4175b9f into dev Apr 2, 2024
5 checks passed
@tisfeng tisfeng deleted the swift-openai branch April 2, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants