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

Simplifying the client API and refactoring federation #268

Merged
merged 6 commits into from
May 3, 2024

Conversation

calvinrp
Copy link
Collaborator

@calvinrp calvinrp commented May 2, 2024

This PR starts to simplify the client API and enables using federation in importing crates such as cargo-component and wac.

Draft PR for cargo-component
Draft PR for wac

@calvinrp calvinrp requested review from lann and macovedj May 2, 2024 21:16
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Only critical question is about interaction in client.

crates/client/src/api.rs Outdated Show resolved Hide resolved
crates/client/src/config.rs Outdated Show resolved Hide resolved
crates/client/src/lib.rs Outdated Show resolved Hide resolved
let package_name = &packages.get(log_id).unwrap().name;

if self.auto_accept_federation_hints
|| Confirm::with_theme(&ColorfulTheme::default())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having this kind of interaction deep in the client like this means it isn't a general purpose client any more, it is specialized to CLIs. Given recent discussions should we give up on this crate being a general-purpose client library?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this crate is only being used for CLIs at the moment. I was leaning towards simplifying this crate for that use case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lann Fine as is? Feature flag?

If refactored into a callback, it would need be async. Started looking complex and wasn't sure it was worthwhile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be refactored later.

crates/client/src/lib.rs Outdated Show resolved Hide resolved
crates/client/src/api.rs Outdated Show resolved Hide resolved
@calvinrp calvinrp merged commit 85f8ea1 into main May 3, 2024
6 checks passed
@calvinrp calvinrp deleted the clientside-federation-refactor branch May 3, 2024 20:55
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