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 SimilarityIndex initialization sync and setupDimension method public #35

Closed
wants to merge 3 commits into from

Conversation

buhe
Copy link
Contributor

@buhe buhe commented Feb 13, 2024

Hi, @ZachNagengast
Constructor as async is not very convenient most of the time

BR
buhe

@ZachNagengast ZachNagengast self-requested a review February 13, 2024 04:36
Copy link
Owner

@ZachNagengast ZachNagengast left a comment

Choose a reason for hiding this comment

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

I definitely see your point that async is sometimes hard to work with - it would help all the existing code though if this was a secondary convenience init method, rather than a direct change to the default. That may require the use of semaphores to work properly.

Another approach you might want to consider is an entirely new class that is a subclass of the SimilarityIndex but has a non-async init, a good reference for how to handle both is the swift-argument-parser project: apple/swift-argument-parser#404

The code looks good otherwise, thanks for adding the documentation line.

@buhe
Copy link
Contributor Author

buhe commented Feb 13, 2024

Okey, i will see apple/swift-argument-parser#404

@buhe buhe closed this Feb 14, 2024
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.

2 participants