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

Implement write_to_disk for uri backends #164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Jun 25, 2019

What was wrong?

Writing resolved ipfs uris to disk is something I find myself doing a lot manually, and would be useful if available on uri backends.

Cute Animal Picture

image

@njgheorghita njgheorghita changed the title Implement write_to_disk for ipfs backends Implement write_to_disk for uri backends Jun 25, 2019
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

This doesn't look like the right API seeing as the same logic is repeated in both IPFS and Github backends. Maybe this should be a singular utility function that takes a backend, a URI and the target path and does the writing to disk?

Can you show me more of how this would be used in the core library API?

@njgheorghita
Copy link
Contributor Author

Yeah, that's a fair point. I'm not against moving this logic to a util function. I was just thinking it might be cleaner/simpler since you already have the IPFS backend to do something like

ipfs_backend.write_to_disk(uri, target_path)

rather than

import write_uri_to_disk
write_uri_to_disk(ipfs_backend, uri, target_path)

In terms of duplicating the logic, I just moved the definition of write_to_disk to the BaseURIBackend, but if you think this is still better off as a utility function, I can see that point of view.

@njgheorghita
Copy link
Contributor Author

njgheorghita commented Jun 27, 2019

I guess this relates to how we interact with the ipfs backend in general. Typically, I find myself instantiating a specific backend (i.e. InfuraIPFSBackend / LocalIPFSBackend) and using them directly. But with the current update to resolve_uri_contents(uri) -> contents which asynchronously automatically tries all the backends to fetch the contents, it might make sense to have a similar function write_uri_to_disk(uri, target_path) which is backend agnostic and just tries to fetch from whatever backend is able/available. Encouraging users to skip instantiating their own URI backend to fetch contents and use the utils resolve_uri_contents() / write_uri_to_disk.

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.

2 participants