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

Make info command return ServerState struct #60

Closed
rustworthy opened this issue Apr 10, 2024 · 4 comments
Closed

Make info command return ServerState struct #60

rustworthy opened this issue Apr 10, 2024 · 4 comments

Comments

@rustworthy
Copy link
Collaborator

Info command will now return a result with serde_json::Value which made a lot of sense during the period when the Faktory library was being actively developed and so the shape of the return type could change breakingly. The last time when that interface was updated was a couple of years ago and so we may want to introduce a dedicated type for it to make Client::info more ergonomic:

let server_state = client.info().await.unwrap(); // ServerState
let queues = &server_state.faktory.queues; // &HashMap<String, u64>

While currently - provided they know the keys - this will look like:

let server_state = client.info().await.unwrap(); // serde_json::Value
let queues = server_state.get("faktory").unwrap().get("queues").unwrap(); // &serde_json::Value

An example usage of Client::info can be found here

Since a breaking change is occurring due to switch to async, the Client::info can be updated to return ServerState.

@jonhoo
Copy link
Owner

jonhoo commented Apr 27, 2024

That seems reasonable! If we also annotate it with #[non_exhaustive], we'll even be able to expose more fields if they should be added.

@mperham
Copy link

mperham commented Apr 27, 2024

Related, see my recent Faktory PR which adds a typed structure for INFO.

@jonhoo
Copy link
Owner

jonhoo commented Apr 28, 2024

Ah, this is contribsys/faktory#474?

Given we're doing a breaking change as part of #49 anyway, I think we should probably just remove .info entirely and add .current_info that returns a type matching the one you added here:
https://github.com/contribsys/faktory/pull/474/files#diff-4bbbde2a77215c352f9e99f0f2764fb9fb09eaa9052433b58a474b33019f7486R9

@rustworthy
Copy link
Collaborator Author

Ah, this is contribsys/faktory#474?

Given we're doing a breaking change as part of #49 anyway, I think we should probably just remove .info entirely and add .current_info that returns a type matching the one you added here: https://github.com/contribsys/faktory/pull/474/files#diff-4bbbde2a77215c352f9e99f0f2764fb9fb09eaa9052433b58a474b33019f7486R9

Addressed in #59

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

No branches or pull requests

3 participants