-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add PackageSearchClient
#5950
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
Add PackageSearchClient
#5950
Conversation
@swift-ci please smoke test |
Example results:
|
345c97e
to
2677d2a
Compare
@swift-ci please smoke test |
|
||
public func findPackages( | ||
_ query: String, | ||
callback: @escaping (Result<[Package], Error>) -> Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to have all new apis adopt async/await if we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but considering the dire state of async/await on CI, I'm not sure if we should proliferate it, yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what are you referring to by "state of async/await on CI"? I was hoping I could try using it in some of the new experimental-destination
subcommands, especially after we update ArgumentParser 1.1.4 where AsyncParsableCommand
is available. Is there anything I should be aware before I even consider it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use it at all on macOS CI so far, in case that is necessary, we should avoid it. As long as we can scope the use so that macOS CI still works but just doesn't test a certain feature, that should be fine in my book.
location: $0.location, | ||
versions: $0.versions.map { $0.version }, | ||
readmeURL: $0.readmeURL) } | ||
if packages.isEmpty, let error = error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the requirement on both is a bit odd? but perhaps i am missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to normally report the success for the empty search result, but if we don't get a result via search to provide any previous error. So if the query was a registry ID or URL that caused an error which also did not yield any results through search, we'd see the underlying error.
one other thing to add here is that the result can include where (which provider) the metadata came from, I think that can be useful in a UI |
@swift-ci please smoke test |
Looks like 5.6 needs a bit more help:
|
This offers a generic interface which allows clients to find packages via exact matching of registry identities or URLs, as well as search through collections and index if there are no exact matches.
2677d2a
to
28ec83e
Compare
@swift-ci please smoke test |
ugh, looks like our good friend is back :/
|
@swift-ci please smoke test macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to let @compnerd know whenever there is a new module?
Other than that, LGTM
@swift-ci please smoke test windows |
Looks like the Windows job wasn't run previously? |
Windows error looks unrelated:
|
@swift-ci please smoke test windows |
@swift-ci please smoke test windows |
Was looking into adding the new module to the installer scripts, but actually I am not adding it to the CMake build at all for now, so this should be fine as-is. |
As long as nothing is linking against it and it doesn't diverge functionality, I think that should be fine. But we should keep the platforms in parity - so if we are adding a new module, we should be adding it to the CMake build and the swift-installer-scripts packaging IMO. |
I agree with that, but I see this particular change as an extension of the package-collections functionality which was never available on Windows to begin with (probably by mistake). We should address this, but separately from this PR. |
This offers a generic interface which allows clients to find packages via exact matching of registry identities or URLs, as well as search through collections and index if there are no exact matches.