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

Discussion: Backwards incompatible interface change #44

Closed
melroy89 opened this issue Mar 31, 2021 · 2 comments
Closed

Discussion: Backwards incompatible interface change #44

melroy89 opened this issue Mar 31, 2021 · 2 comments
Assignees

Comments

@melroy89
Copy link
Collaborator

melroy89 commented Mar 31, 2021

Hi,

I noticed you wrapped the files cat API command with the method name FilesGet:

void Client::FilesGet(const std::string& path, std::iostream* response) {
  http_->Fetch(MakeUrl("cat", {{"arg", path}}), {}, response);
}

Which will get the file via cat and returns the data via iostream as shown above.

However, FilesGet call will conflict with the actual get HTTP API call as well:

https://docs.ipfs.io/reference/http/api/#api-v0-get

Proposal: Fix the misalignment of interface names better sooner than later. Rename FilesGet to FilesCat. And FilesGet can be implemented to download the file directly to disk (proving the arg as path argument).
This will be conform the spec and even in-line with the API CLI.

This change will however impact devs who upgrade to a newer version of cpp-ipfs-http-client, it isn't nice. But again, I rather do this change now then later.

An alternative is: Keep the current names. And add FilesDownload to use the get API call. Which is, as expected, not inline with the spec.

@vasild Please, let me know what you think.

Regards,
Melroy van den Berg

@vasild
Copy link
Owner

vasild commented Apr 1, 2021

The thing is that cat implies printing the file to stdout which makes sense from the perspective of a cli tool, but not from the perspective of a library (or does it?).

So I think it is most reasonable to have a method that retrieves the file and provides its contents to the caller (like FilesGet does now).

Maybe another method to "download" the file - store its contents on disk instead of returning it to the caller.

If there is a method named FilesCat I think it should print the contents of the file to stdout. Maybe this would not be very useful, this is why I did not add it.

@melroy89
Copy link
Collaborator Author

melroy89 commented Apr 1, 2021

OK. Thank you for your reply.

Clear story. So let's keep the current naming convention. And maybe add filesdownload

@melroy89 melroy89 closed this as completed Apr 1, 2021
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

2 participants