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

[suggestion] Let list methods take an Option argument #243

Open
vrurg opened this issue Jul 12, 2024 · 5 comments
Open

[suggestion] Let list methods take an Option argument #243

vrurg opened this issue Jul 12, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@vrurg
Copy link
Contributor

vrurg commented Jul 12, 2024

Maybe this is my lack of experience, but when I'm requesting just a full list of objects like files I have to write something like:

    let file_list = files.list::<[&str; 0]>(&[]).await?;

Wouldn't it be better to let it be just files.list(None).await??

@64bit
Copy link
Owner

64bit commented Jul 12, 2024

Thanks for suggestion, it can be improved, how about having two functions

  1. files.list() with no parameters, which calls another public function below
  2. files.list_with_query(query: Q)

I think files.list() has better ergonomics than files.list(None).

@64bit 64bit added the enhancement New feature or request label Jul 12, 2024
@vrurg
Copy link
Contributor Author

vrurg commented Jul 13, 2024

I think files.list() has better ergonomics than files.list(None).

I agree. I just followed #167 and tried to figure out a good compromise.

BTW, a side note here. The query arguments are currently undocumented (vector store list method uses the same design). Perhaps it worth pointing out at reqwest crate for those looking for understanding without digging into the sources, like I did.

@64bit
Copy link
Owner

64bit commented Jul 14, 2024

That's a good observation too about lack of documentation on query parameter - any contribution to help improve it is most welcome! (I think one of examples have a usage but that's not very visible/searchable on docs.rs)

Discussion in #167 was in context of missing the call for Files api group. It does seem like it needs to be addressed across the whole crate, that does sound like a breaking change where existing list becomes list_with_query

Its a small breaking-change and and an improvement so why not? Can be released in a version bump.

In addition, breaking change is ok too because list would wrap list_with_query so easy to maintain and implement.

@vrurg
Copy link
Contributor Author

vrurg commented Jul 17, 2024

That's a good observation too about lack of documentation on query parameter - any contribution to help improve it is most welcome! (I think one of examples have a usage but that's not very visible/searchable on docs.rs)

Oh, I know this tone... :) Unfortunately, as you can see, I barely find time on side-projects now. Had to even quit Raku development. Otherwise it'd be my pleasure!

@64bit
Copy link
Owner

64bit commented Jul 18, 2024

No obligations, thank you for your contributions, the issue is going to be here if you wanna give it a shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants