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

Improved Crawly.fetch/2 to accept fetching with a spider #107

Merged
merged 14 commits into from Nov 13, 2020

Conversation

Ziinc
Copy link
Collaborator

@Ziinc Ziinc commented May 18, 2020

for #52, implements a :with option that allows the passing of the spider name.

Breaking changes includes moving of the headers and options parameters to the opts parameter, as keyword lists instead of positional args.

@Ziinc Ziinc changed the title Added with option, moved all optional parameters to list opt Improved Crawly.fetch/2 May 18, 2020
@Ziinc Ziinc changed the title Improved Crawly.fetch/2 Improved Crawly.fetch/2 to accept fetching with a spider May 18, 2020
@Ziinc Ziinc requested a review from oltarasenko May 18, 2020 09:52
@Ziinc
Copy link
Collaborator Author

Ziinc commented Nov 2, 2020

Updated, tests passes on my side, should be ok to merge.

@oltarasenko
Copy link
Collaborator

Looks extremely good. The question is: "Can we move it outside of the main module? E.g. I want to shrink the crawly.ex"

@Ziinc could you fix the test, alternatively, I can pick it up if you don't have time right now.

@Ziinc
Copy link
Collaborator Author

Ziinc commented Nov 11, 2020

We could move to Utils, but Utils is also getting quite fat

Ziinc and others added 2 commits November 11, 2020 23:11
* Add support of initial arguments

Implemented as a part of #126 feature request

* updated tests, added docs

Co-authored-by: Ziinc <ty@tzeyiing.com>
@oltarasenko
Copy link
Collaborator

Yeah. I have the same feelings. I was thinking of splitting parts related to Crawly from parts related to spiders into separate modules.

Maybe it could make these modules small. In any case, can be done later on separately. Could you rebase it, so I will check again.

@oltarasenko
Copy link
Collaborator

@Ziinc I am about to cut off a new release. Could you please rebase this change, so I can include it as well?

@Ziinc
Copy link
Collaborator Author

Ziinc commented Nov 13, 2020

Done!

@oltarasenko oltarasenko merged commit 7684b35 into master Nov 13, 2020
@oltarasenko oltarasenko deleted the 52-improve-fetch-with-option branch November 13, 2020 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants