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

Separate download func to smaller units #19

Closed
nappex opened this issue Dec 30, 2021 · 6 comments · Fixed by #72
Closed

Separate download func to smaller units #19

nappex opened this issue Dec 30, 2021 · 6 comments · Fixed by #72

Comments

@nappex
Copy link
Collaborator

nappex commented Dec 30, 2021

We'd like to divide Onigumo.download to smaller parts with philosophy one func make one task.
This approach is more suitable for deep testing also. We'd like to use tmp_dir in test for testing files, for this intention we need to have the way how to give filepath as argument. When we reach it, we will be able to give tmp_dir as argument to make integration tests.
Onigumo.download make three different tasks at this moment.

Three tasks are:

  1. make request
  2. check request and match body of request to variable
  3. write body content to file
@Glutexo
Copy link
Owner

Glutexo commented Dec 30, 2021

Cool!

@Glutexo
Copy link
Owner

Glutexo commented Jan 21, 2022

Partially addressed by #29.

@Glutexo
Copy link
Owner

Glutexo commented Feb 7, 2022

Now with #29 merged, we can think about whether and how to split the download function to smaller, more atomic, function. I am a bit afraid of having too small chunks, but that’s probably the real functional way. Until now, every split has proven itself useful and good for testing.

@Glutexo
Copy link
Owner

Glutexo commented Feb 13, 2022

Streams would help this a lot. See #70.

@Glutexo
Copy link
Owner

Glutexo commented Feb 13, 2022

Partially addressed by #72. I am not sure yet how to split out the body extraction from the response object.

@Glutexo
Copy link
Owner

Glutexo commented Feb 19, 2022

#72 eventually resolved this completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants