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

String value for order of Scrapy component #2420

Closed
vshlapakov opened this issue Dec 1, 2016 · 5 comments
Closed

String value for order of Scrapy component #2420

vshlapakov opened this issue Dec 1, 2016 · 5 comments
Labels

Comments

@vshlapakov
Copy link

@vshlapakov vshlapakov commented Dec 1, 2016

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.

@redapple
Copy link
Contributor

@redapple redapple commented Dec 1, 2016

Right, it's probably not stated explicitly nor checked, but the examples given in the docs use integers as far as I can see.
In which case did you see this bug @vshlapakov ?

@vshlapakov
Copy link
Author

@vshlapakov vshlapakov commented Dec 1, 2016

@redapple Yeah, that's a not too frequent case to worry about, but I found it in our own internal project (it's linked with this issue on above). It's fixed there already, but someone can do the same mistake.

@redapple redapple added the help wanted label Dec 6, 2016
@elacuesta
Copy link
Member

@elacuesta elacuesta commented Dec 15, 2016

How about something like

for name, value in six.iteritems(compdict):
    if value is not None and not isinstance(value, int):
        try:
            compdict[name] = int(value.strip())
        except:
            raise ValueError('Invalid value {} for component {}, please ' \
                             'provide an integer instead'.format(value, name))

somewhere in build_component_list?

I could open a PR with some mention in the docs and some tests if you like this approach.

@vshlapakov
Copy link
Author

@vshlapakov vshlapakov commented Dec 21, 2016

@elacuesta Makes sense to me, that would be nice, thanks. In my case I encountered with a string with an integer inside as order, so it should be covered by the similar changes. Btw there's no need to strip value before converting to int, but that's nitpicking :)

@elacuesta
Copy link
Member

@elacuesta elacuesta commented Dec 23, 2016

PR created ☝️
Thanks for your comment Viktor :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.