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

Convert load_urls to a Stream #66

Merged
merged 3 commits into from
Feb 19, 2022
Merged

Convert load_urls to a Stream #66

merged 3 commits into from
Feb 19, 2022

Conversation

Glutexo
Copy link
Owner

@Glutexo Glutexo commented Feb 12, 2022

The downloading should work as a stream, everything from picking the URL from the file through the download from the Internet to the local drive file write should happen separately for every item, not by repeated enumeration over a list. This required another version of the download/3 function accepting a Stream and returning a Stream. Started with converting the load_urls/1 function to a Stream.

The downloading should work as a stream, everything from picking the
URL from the file through the download from the Internet to the local
drive file write should happen separately for every item, not by
repeated enumeration over a list. This required another version of the
download/3 function accepting a Stream and returning a Stream.
@Glutexo Glutexo requested a review from nappex February 12, 2022 21:44
@Glutexo Glutexo self-assigned this Feb 12, 2022
@Glutexo
Copy link
Owner Author

Glutexo commented Feb 12, 2022

I now think that the download/3 function that accepts a list of URLs is unnecessary. Traversing an enumerable is a trivial operation and we don’t need to abstract it and separately test it. Here, with the introduction of streams, it shows us that there are more ways to call the download/3 function with a single URL and until now, it was only limited to lists.

@Glutexo Glutexo mentioned this pull request Feb 12, 2022
@Glutexo
Copy link
Owner Author

Glutexo commented Feb 12, 2022

The aforementioned is addressed in #67.

@Glutexo
Copy link
Owner Author

Glutexo commented Feb 13, 2022

Merged current elixir and resolved conflicts.

@Glutexo
Copy link
Owner Author

Glutexo commented Feb 13, 2022

Merged current elixir and resolved conflicts. I did not want to duplicate the download_urls/3 function now when it contains the file name functionality. It stroke me as a bad piece of design. Thus only kept the load_urls/1 method conversion with no further changes.

@nappex nappex merged commit 07b6578 into elixir Feb 19, 2022
@nappex nappex deleted the stream branch February 19, 2022 19:39
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.

None yet

2 participants