Repository: scrapy/scrapy

Issue:
Request doesn't set callback to self.parse automatically when errback is set
Hi, I'm facing an `AssertionError` when set `errback`  and not `callback` in `Request`, is that expected? :

```
...
    def start_requests(self):
        yield Request(
            self.start_url,
            # callback=self.parse,
            errback=self.parse_error,
            meta={
                'dont_redirect': True,
            },
        )
...
```
trigger this : 
```
2019-01-16 15:47:22 [scrapy.core.engine] ERROR: Error while obtaining start requests
Traceback (most recent call last):
  File "/home/joaquin/Repos/example/env/lib/python3.6/site-packages/scrapy/core/engine.py", line 127, in _next_request
    request = next(slot.start_requests)
  File "/home/joaquin/Repos/example/example/spiders/br/rj/example.py", line 128, in start_requests
    'dont_redirect': True,
  File "/home/joaquin/Repos/example/env/lib/python3.6/site-packages/scrapy/http/request/__init__.py", line 34, in __init__
    assert callback or not errback, "Cannot use errback without a callback"
AssertionError: Cannot use errback without a callback
2019-01-16 15:47:22 [scrapy.core.engine] INFO: Closing spider (finished)
2019-01-16 15:47:22 [scrapy.core.engine] INFO: Spider closed (finished)
```

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/4008?src=pr&el=h1) Report
> Merging [#4008](https://codecov.io/gh/scrapy/scrapy/pull/4008?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/6ad5a89cb0aeac18a72ed41819c07e9c22990f10?src=pr&el=desc) will **decrease** coverage by `0.24%`.
> The diff coverage is `n/a`.


```diff
@@            Coverage Diff             @@
##           master    #4008      +/-   ##
==========================================
- Coverage   85.68%   85.43%   -0.25%     
==========================================
  Files         165      167       +2     
  Lines        9734     9732       -2     
  Branches     1463     1456       -7     
==========================================
- Hits         8341     8315      -26     
- Misses       1136     1159      +23     
- Partials      257      258       +1
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/4008?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/extensions/corestats.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2V4dGVuc2lvbnMvY29yZXN0YXRzLnB5) | `90% <0%> (-10%)` | :arrow_down: |
| [scrapy/core/scraper.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvc2NyYXBlci5weQ==) | `86.48% <0%> (-2.85%)` | :arrow_down: |
| [scrapy/commands/runspider.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL3J1bnNwaWRlci5weQ==) | `74.28% <0%> (-1.08%)` | :arrow_down: |
| [scrapy/pipelines/files.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L3BpcGVsaW5lcy9maWxlcy5weQ==) | `65.38% <0%> (-0.79%)` | :arrow_down: |
| [scrapy/spiders/crawl.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L3NwaWRlcnMvY3Jhd2wucHk=) | `81.7% <0%> (-0.44%)` | :arrow_down: |
| [scrapy/item.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2l0ZW0ucHk=) | `98.55% <0%> (-0.12%)` | :arrow_down: |
| [scrapy/core/engine.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZW5naW5lLnB5) | `92.76% <0%> (-0.04%)` | :arrow_down: |
| [scrapy/logformatter.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L2xvZ2Zvcm1hdHRlci5weQ==) | `90.47% <0%> (ø)` | :arrow_up: |
| [scrapy/xlib/pydispatch.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L3hsaWIvcHlkaXNwYXRjaC5weQ==) | `0% <0%> (ø)` | |
| [scrapy/xlib/tx.py](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree#diff-c2NyYXB5L3hsaWIvdHgucHk=) | `0% <0%> (ø)` | |
| ... and [1 more](https://codecov.io/gh/scrapy/scrapy/pull/4008/diff?src=pr&el=tree-more) | |

elacuesta: Tests are green without the restriction (the one failure is because of #4014, it can be ignored IMHO).
@pablohoffman since you authored the commit that introduced the assertion, do you have any objections to remove it?
pablohoffman: no objections @elacuesta!
Gallaecio: > Tests are green without the restriction

Unless we already have it, though, this might be a great opportunity to add a test for this, that specifically verifies that a callback can be omitted even if an errback is provided.

Patch:
diff --git a/scrapy/http/request/__init__.py b/scrapy/http/request/__init__.py
index 76a428199..61c0a4c9e 100644
--- a/scrapy/http/request/__init__.py
+++ b/scrapy/http/request/__init__.py
@@ -32,7 +32,6 @@ class Request(object_ref):
             raise TypeError('callback must be a callable, got %s' % type(callback).__name__)
         if errback is not None and not callable(errback):
             raise TypeError('errback must be a callable, got %s' % type(errback).__name__)
-        assert callback or not errback, "Cannot use errback without a callback"
         self.callback = callback
         self.errback = errback
 


Test patch:
diff --git a/tests/test_http_request.py b/tests/test_http_request.py
index 9df6ff67b..3449c7a40 100644
--- a/tests/test_http_request.py
+++ b/tests/test_http_request.py
@@ -246,25 +246,41 @@ class RequestTest(unittest.TestCase):
         self.assertRaises(AttributeError, setattr, r, 'url', 'http://example2.com')
         self.assertRaises(AttributeError, setattr, r, 'body', 'xxx')
 
-    def test_callback_is_callable(self):
+    def test_callback_and_errback(self):
         def a_function():
             pass
-        r = self.request_class('http://example.com')
-        self.assertIsNone(r.callback)
-        r = self.request_class('http://example.com', a_function)
-        self.assertIs(r.callback, a_function)
-        with self.assertRaises(TypeError):
-            self.request_class('http://example.com', 'a_function')
 
-    def test_errback_is_callable(self):
-        def a_function():
-            pass
-        r = self.request_class('http://example.com')
-        self.assertIsNone(r.errback)
-        r = self.request_class('http://example.com', a_function, errback=a_function)
-        self.assertIs(r.errback, a_function)
+        r1 = self.request_class('http://example.com')
+        self.assertIsNone(r1.callback)
+        self.assertIsNone(r1.errback)
+
+        r2 = self.request_class('http://example.com', callback=a_function)
+        self.assertIs(r2.callback, a_function)
+        self.assertIsNone(r2.errback)
+
+        r3 = self.request_class('http://example.com', errback=a_function)
+        self.assertIsNone(r3.callback)
+        self.assertIs(r3.errback, a_function)
+
+        r4 = self.request_class(
+            url='http://example.com',
+            callback=a_function,
+            errback=a_function,
+        )
+        self.assertIs(r4.callback, a_function)
+        self.assertIs(r4.errback, a_function)
+
+    def test_callback_and_errback_type(self):
+        with self.assertRaises(TypeError):
+            self.request_class('http://example.com', callback='a_function')
         with self.assertRaises(TypeError):
-            self.request_class('http://example.com', a_function, errback='a_function')
+            self.request_class('http://example.com', errback='a_function')
+        with self.assertRaises(TypeError):
+            self.request_class(
+                url='http://example.com',
+                callback='a_function',
+                errback='a_function',
+            )
 
     def test_from_curl(self):
         # Note: more curated tests regarding curl conversion are in