Add an input URL list parameter #38
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
+ Coverage 98.81% 98.90% +0.09%
==========================================
Files 12 12
Lines 506 549 +43
==========================================
+ Hits 500 543 +43
Misses 6 6
|
Gallaecio
changed the title
| """ | ||
| if isinstance(value, str): | ||
| value = value.split("\n") | ||
| if value: |
There was a problem hiding this comment.
What we expect to have is it's not str instance?
There was a problem hiding this comment.
Any of the actually supported types: None or a list of str.
There was a problem hiding this comment.
@Gallaecio will you add something to make this work?
mind that "URL\nURL" will not work - from your comment in description.
or running like
scrapy crawl spider -a urls="https://www.nytimes.com/\nhttps://www.theguardian.com"
I've added this workaround https://github.com/zytedata/zyte-spider-templates-private/blob/article/zyte_spider_templates/spiders/article.py#L135, but might be we can handle it in validate_url_list too.
There was a problem hiding this comment.
I think supporting that syntax is a bit dangerous, as \n are valid URL characters. Since it will not be an issue in the UI or from code, I am not sure if it is worth supporting.
Maybe we could support spaces as a separator, but we risk scenarios were users expect a space → %20 conversion to happen.
There was a problem hiding this comment.
@Gallaecio I've checked https://www.rfc-editor.org/rfc/rfc3986 and haven't found anything about \n as allowed character - but might be missed something.
There was a problem hiding this comment.
I meant valid characters, i.e. \ is a valid character and n is a valid character. https://example.com/\n is a valid URL.
| @model_validator(mode="after") | ||
| def single_input(self): | ||
| """Fields | ||
| :class:`~zyte_spider_templates.spiders.ecommerce.EcommerceSpiderParams.url` |
There was a problem hiding this comment.
If it's base class and we are going to use the same approach in Articles - in this case, might be no need to mention ecommerce.EcommerceSpiderParams explicitly.
There was a problem hiding this comment.
Once we merge article support, we can refactor the docs to cover shared params elsewhere in the docs, and point there. But for the current state of the repository, this is the best thing we can link to.
There was a problem hiding this comment.
LGTM. A few thoughts:
Making this available in the ecommerce template might mean that the upcoming article spider could be released without the max_requests_per_seed parameter. We can do it after the article launch. What do you think @PyExplorer ?
This parameter is quite important for Articles, especially for incremental crawl - discussed here https://zytegroup.slack.com/archives/G011GR9M47N/p1712058449481609?thread_ts=1712047414.333929&cid=G011GR9M47N |
|
Is there anything left to do in this PR, besides resolving the conflicts? |
Addresses the first half of #36. (I plan to have a separate PR for the ability to specify input URLs that point to the actual start URLs).
To do:
\nas URL separator,separatesame spider)Tested manually. When specifying the
urlsparameter from a terminal, mind that "URL\nURL" will not work.