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

[MRG] make scrapy.version_info a tuple of integers #681

Merged
merged 1 commit into from Apr 14, 2014

Conversation

@kmike
Copy link
Member

@kmike kmike commented Apr 5, 2014

sys.version_info[:3] is a tuple of ints, and sqlite.version_info is a tuple of ints, but scrapy.version_info is a tuple of strings now. It may cause two subtle errors:

  1. when comparing with an another tuple of strings values are compared alphabetically, so "3" is greater than "22";
  2. when comparing with a tuple of ints in Python 2.x it always returns False.

One nice thing about Python 3 porting is that it exposes this kind of errors :)
Tests in #679 don't run under Python 3.3 partially because of this, see https://travis-ci.org/scrapy/scrapy/jobs/22299093.

@dangra
Copy link
Member

@dangra dangra commented Apr 10, 2014

+1 I think it is fine to break backward compatibility in this case, I can't see any other way.

My only doubt is if it is worth using a NamedTuple to mimic sys.version_info as implemented in http://bugs.python.org/issue4285

we can at least add attribute access for major, minor and micro values.

dangra added a commit that referenced this pull request Apr 14, 2014
[MRG] make scrapy.version_info a tuple of integers
@dangra dangra merged commit 0a7e655 into scrapy:master Apr 14, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@kmike
Copy link
Member Author

@kmike kmike commented Apr 14, 2014

I think that there is nothing wrong with version_info being namedtuple, but it doesn't matter much as it is rarely used.

@kmike kmike deleted the kmike:version_info branch Mar 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants