Repository: scrapy/scrapy

Issue:
String value for order of Scrapy component
If Scrapy component order is defined as a string, it leads to undefined behaviour on Python 2 and to the following errors on Python 3:
```
File "/usr/local/lib/python3.5/site-packages/scrapy/middleware.py", line 58, in from_crawler
 return cls.from_settings(crawler.settings, crawler)
File "/usr/local/lib/python3.5/site-packages/scrapy/middleware.py", line 29, in from_settings
 mwlist = cls._get_mwlist_from_settings(settings)
File "/usr/local/lib/python3.5/site-packages/scrapy/core/spidermw.py", line 21, in _get_mwlist_from_settings
 return build_component_list(settings.getwithbase('SPIDER_MIDDLEWARES'))
File "/usr/local/lib/python3.5/site-packages/scrapy/utils/conf.py", line 47, in build_component_list
 return [k for k, v in sorted(six.iteritems(compdict), key=itemgetter(1))]
builtins.TypeError: unorderable types: str() < int()
```

My guess that 1) order of a Scrapy component should be stated as of integer type (or `None`) and there should be a check somewhere, 2) or the sorting logic should be fixed.

Discussion:
codecov-io: ## [Current coverage](https://codecov.io/gh/scrapy/scrapy/pull/2464?src=pr) is 83.46% (diff: 100%)
> Merging [#2464](https://codecov.io/gh/scrapy/scrapy/pull/2464?src=pr) into [master](https://codecov.io/gh/scrapy/scrapy/branch/master?src=pr) will increase coverage by **0.01%**

```diff
@@             master      #2464   diff @@
==========================================
  Files           161        161          
  Lines          8779       8787     +8   
  Methods           0          0          
  Messages          0          0          
  Branches       1288       1291     +3   
==========================================
+ Hits           7326       7334     +8   
  Misses         1205       1205          
  Partials        248        248          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [cc06b6b...f83a45f](https://codecov.io/gh/scrapy/scrapy/compare/cc06b6b1f68d33c1a5e5429d6dfb9b39577624fe...f83a45fa5ffff797601c0e1b20acec3fd4caea1d?src=pr)
kmike: Did float priorities work before this change? They look useful.
elacuesta: They worked, and now that I took a closer look the documentation doesn't actually say that the numbers should be integers. I'll change it to allow floats too, thanks for noticing @kmike!
elacuesta: @redapple, @kmike The branch is still named `component_order_integer`, would you guys like me to close this PR and open a new one with a more suitable branch name?
redapple: @elacuesta , that branch name is not an issue, don't worry
redapple: Still looks good to me.
@dangra , @kmike , @vshlapakov, what do you think?
kmike: This looks good! 

May I ask for the last change? :) `_ensure_numeric_value` name is a bit misleading; could we name it `_ensure_numeric_values` or `_raise_if_unsupported_values` or `_validate_values`, something like that? `value` -> `values` because we're working with dict values; `ensure` -> `validate / ...` because code no longer converts strings to numbers.
elacuesta: @kmike Updated, I went with `_validate_numeric_values` :-)
vshlapakov: Looks good, thanks @elacuesta! 
kmike: Hey @elacuesta! Sorry for being so picky. I'm not a native speaker, but for me `_validate_numeric_values` reads as "validate values if they are numeric", i.e. "check only numeric values, but not other values - e.g. ensure that they are positive", not "check that all values are numeric" :)

Patch:
diff --git a/scrapy/utils/conf.py b/scrapy/utils/conf.py
index e8af90f11..435e9a6b3 100644
--- a/scrapy/utils/conf.py
+++ b/scrapy/utils/conf.py
@@ -1,5 +1,6 @@
 import os
 import sys
+import numbers
 from operator import itemgetter
 
 import six
@@ -34,6 +35,13 @@ def build_component_list(compdict, custom=None, convert=update_classpath):
             _check_components(compdict)
             return {convert(k): v for k, v in six.iteritems(compdict)}
 
+    def _validate_values(compdict):
+        """Fail if a value in the components dict is not a real number or None."""
+        for name, value in six.iteritems(compdict):
+            if value is not None and not isinstance(value, numbers.Real):
+                raise ValueError('Invalid value {} for component {}, please provide ' \
+                                 'a real number or None instead'.format(value, name))
+
     # BEGIN Backwards compatibility for old (base, custom) call signature
     if isinstance(custom, (list, tuple)):
         _check_components(custom)
@@ -43,6 +51,7 @@ def build_component_list(compdict, custom=None, convert=update_classpath):
         compdict.update(custom)
     # END Backwards compatibility
 
+    _validate_values(compdict)
     compdict = without_none_values(_map_keys(compdict))
     return [k for k, v in sorted(six.iteritems(compdict), key=itemgetter(1))]
 


Test patch:
diff --git a/tests/test_utils_conf.py b/tests/test_utils_conf.py
index dab41ac8d..f203c32ef 100644
--- a/tests/test_utils_conf.py
+++ b/tests/test_utils_conf.py
@@ -62,6 +62,27 @@ class BuildComponentListTest(unittest.TestCase):
         self.assertRaises(ValueError, build_component_list, duplicate_bs,
                           convert=lambda x: x.lower())
 
+    def test_valid_numbers(self):
+        # work well with None and numeric values
+        d = {'a': 10, 'b': None, 'c': 15, 'd': 5.0}
+        self.assertEqual(build_component_list(d, convert=lambda x: x),
+                         ['d', 'a', 'c'])
+        d = {'a': 33333333333333333333, 'b': 11111111111111111111, 'c': 22222222222222222222}
+        self.assertEqual(build_component_list(d, convert=lambda x: x),
+                         ['b', 'c', 'a'])
+        # raise exception for invalid values
+        d = {'one': '5'}
+        self.assertRaises(ValueError, build_component_list, {}, d, convert=lambda x: x)
+        d = {'one': '1.0'}
+        self.assertRaises(ValueError, build_component_list, {}, d, convert=lambda x: x)
+        d = {'one': [1, 2, 3]}
+        self.assertRaises(ValueError, build_component_list, {}, d, convert=lambda x: x)
+        d = {'one': {'a': 'a', 'b': 2}}
+        self.assertRaises(ValueError, build_component_list, {}, d, convert=lambda x: x)
+        d = {'one': 'lorem ipsum',}
+        self.assertRaises(ValueError, build_component_list, {}, d, convert=lambda x: x)
+
+
 
 class UtilsConfTestCase(unittest.TestCase):