Repository: scrapy/scrapy
Pull request: deprecate Spider.make_requests_from_url. 
Fixes #1495.

Todo:
- [x] implementation is incorrect; fix it
- [x] tests


Problem statement:
deprecate Spider.make_requests_from_url
Hey,

`Spider.make_requests_from_url` is a shortcut for a single use case: sending initial requests from a spider with start_urls attribute. It saves a single line of code (`for url in start_urls`) in one specific use case, but yet we have to document it, users have to read about it and understand what it is for. 

The name suggests it is more general - but we can't yield Link objects and get requests created by `make_requests_from_url`, it does nothing for CrawlSpider (I'd expect URLs to be passed through this method), and it is ignored for SitemapSpider. This is inconsistent.

What about deprecating `make_requests_from_url` and removing it from docs? IMHO it will make API simpler, this hook does nothing useful now, and if we ever want a general 'process URL and get a Request' method we'd have to use another name for backwards compatibility anyways.

Hints from issue discussion:
I agree; this method isn't that useful. Also now you even have a shorter import for `Request` if you wanna be lazy (:

+1

Commit message: deprecate Spider.make_requests_from_url. Fixes #1495.

Patch to review:
diff --git a/docs/topics/spiders.rst b/docs/topics/spiders.rst
index c123c2635a0..5e69055d1aa 100644
--- a/docs/topics/spiders.rst
+++ b/docs/topics/spiders.rst
@@ -144,16 +144,12 @@ scrapy.Spider
    .. method:: start_requests()
 
        This method must return an iterable with the first Requests to crawl for
-       this spider.
+       this spider. It is called by Scrapy when the spider is opened for
+       scraping. Scrapy calls it only once, so it is safe to implement
+       :meth:`start_requests` as a generator.
 
-       This is the method called by Scrapy when the spider is opened for
-       scraping when no particular URLs are specified. If particular URLs are
-       specified, the :meth:`make_requests_from_url` is used instead to create
-       the Requests. This method is also called only once from Scrapy, so it's
-       safe to implement it as a generator.
-
-       The default implementation uses :meth:`make_requests_from_url` to
-       generate Requests for each url in :attr:`start_urls`.
+       The default implementation generates ``Request(url, dont_filter=True)``
+       for each url in :attr:`start_urls`.
 
        If you want to change the Requests used to start scraping a domain, this is
        the method to override. For example, if you need to start by logging in using
@@ -172,18 +168,6 @@ scrapy.Spider
                    # each of them, with another callback
                    pass
 
-   .. method:: make_requests_from_url(url)
-
-       A method that receives a URL and returns a :class:`~scrapy.http.Request`
-       object (or a list of :class:`~scrapy.http.Request` objects) to scrape. This
-       method is used to construct the initial requests in the
-       :meth:`start_requests` method, and is typically used to convert urls to
-       requests.
-
-       Unless overridden, this method returns Requests with the :meth:`parse`
-       method as their callback function, and with dont_filter parameter enabled
-       (see :class:`~scrapy.http.Request` class for more info).
-
    .. method:: parse(response)
 
        This is the default callback used by Scrapy to process downloaded
diff --git a/scrapy/spiders/__init__.py b/scrapy/spiders/__init__.py
index c08bb964ab0..138cdbc67de 100644
--- a/scrapy/spiders/__init__.py
+++ b/scrapy/spiders/__init__.py
@@ -66,10 +66,20 @@ def _set_crawler(self, crawler):
         crawler.signals.connect(self.close, signals.spider_closed)
 
     def start_requests(self):
-        for url in self.start_urls:
-            yield self.make_requests_from_url(url)
+        if self.make_requests_from_url is not Spider.make_requests_from_url:
+            warnings.warn(
+                "Spider.make_requests_from_url method is deprecated; "
+                "it won't be called in future Scrapy releases. "
+                "Please override start_requests method instead."
+            )
+            for url in self.start_urls:
+                yield self.make_requests_from_url(url)
+        else:
+            for url in self.start_urls:
+                yield Request(url, dont_filter=True)
 
     def make_requests_from_url(self, url):
+        """ This method is deprecated. """
         return Request(url, dont_filter=True)
 
     def parse(self, response):
diff --git a/tests/spiders.py b/tests/spiders.py
index 711d80cac51..1038b69ded9 100644
--- a/tests/spiders.py
+++ b/tests/spiders.py
@@ -170,10 +170,7 @@ def start_requests(self):
         for i in range(0, self.distinct_urls):
             for j in range(0, self.dupe_factor):
                 url = "http://localhost:8998/echo?headers=1&body=test%d" % i
-                yield self.make_requests_from_url(url)
-
-    def make_requests_from_url(self, url):
-        return Request(url, dont_filter=self.dont_filter)
+                yield Request(url, dont_filter=self.dont_filter)
 
     def __init__(self, url="http://localhost:8998", *args, **kwargs):
         super(DuplicateStartRequestsSpider, self).__init__(*args, **kwargs)
diff --git a/tests/test_engine.py b/tests/test_engine.py
index baf6ef1bff2..04113ddcf6b 100644
--- a/tests/test_engine.py
+++ b/tests/test_engine.py
@@ -66,8 +66,8 @@ def parse_item(self, response):
 
 
 class TestDupeFilterSpider(TestSpider):
-    def make_requests_from_url(self, url):
-        return Request(url)  # dont_filter=False
+    def start_requests(self):
+        return (Request(url) for url in self.start_urls)  # no dont_filter=True
 
 
 class DictItemsSpider(TestSpider):


Code review comments:
diff_hunk: @@ -66,10 +66,20 @@ def _set_crawler(self, crawler):
         crawler.signals.connect(self.close, signals.spider_closed)
 
     def start_requests(self):
-        for url in self.start_urls:
-            yield self.make_requests_from_url(url)
+        if self.make_requests_from_url is not Spider.make_requests_from_url:
path: scrapy/spiders/__init__.py
text: @author:
^^ this line is wrong because the condition is always True: bound method is not the same as unbound.