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

Add python-2.6 support #74

Merged
merged 1 commit into from Feb 24, 2015
Merged

Add python-2.6 support #74

merged 1 commit into from Feb 24, 2015

Conversation

jlaska
Copy link
Contributor

@jlaska jlaska commented Feb 23, 2015

Support python-2.6

  • Adds a tox.ini to facilitate testing supported py_versions
  • Add a requirements-optional-26.txt file
  • Update fauxfactory/init.py to support 2.6 syntax
  • updates tests/test_*.py to conditionally import unittest2
  • Add 2.6 to .travis.yml

@jlaska jlaska mentioned this pull request Feb 23, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9c76729 on jlaska:python26-support into * on omaciel:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9c76729 on jlaska:python26-support into * on omaciel:master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 98cf219 on jlaska:python26-support into * on omaciel:master*.

@jlaska
Copy link
Contributor Author

jlaska commented Feb 23, 2015

Note, the travis failure is caused by a bug in pylint. I can update requirements-optional.txt to use the previous release of pylint.

@@ -57,7 +57,7 @@ def _make_unicode(data):
cast is necessary, a copy of ``data`` is returned.

"""
if sys.version_info.major == 2:
if sys.version_info[0] == 2:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change being made?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have a Python 2.6 system available for manual testing. But the documentation indicates that major, minor, micro, releaselevel, and serial are available. See here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ichimonji10 actually it looks like sys.version_info returns a tuple "containing the five components of the version number: major, minor, micro, releaselevel, and serial" for both 2.6.x and 2.7.x but being able to access these values by name (i.e. sys.version_info.major) can only be done with 2.7.x. If this is indeed the case the change above has merit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering my own question:

Python 2.6.6 (r266:84292, Sep 12 2011, 14:03:14)
[GCC 4.4.5 20110214 (Red Hat 4.4.5-6)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> print sys.version_info
(2, 6, 6, 'final', 0)
>>> print sys.version_info.major
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'tuple' object has no attribute 'major'
>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Thanks for the clarification.

@Ichimonji10
Copy link
Contributor

I found it easier to migrate test collection/execution to pytest rather than conditionally using unitest2 for older python releases.

Conditionally importing unittest should be as easy as this:

from sys import version_info
if version_info.major == 2:
    import unittest2 as unittest
else:
    import unittest

That would eliminate the dependency on tox, pytest and pytest-cov. Also, should tox be listed as a requirement somewhere?

@jlaska
Copy link
Contributor Author

jlaska commented Feb 23, 2015

@Ichimonji10 That's the route I went originally, just disliked having the conditional blog at the top of each module. I can go through route if desired.

Also, I think we only want unittest2 when using <= 2.6 right?

@Ichimonji10
Copy link
Contributor

Also, I think we only want unittest2 when using <= 2.6 right?

Yes. My example code was bad. Something like this is better:

from sys import version_info
if version_info[0:2] == (2, 6):
    import unittest2 as unittest
else:
    import unittest

@jlaska
Copy link
Contributor Author

jlaska commented Feb 23, 2015 via email

@Ichimonji10
Copy link
Contributor

@Ichimonji10 That's the route I went originally, just disliked having the conditional blog at the top of each module. I can go through route if desired.

Fair enough.

My personal preference is that we stick with tools from the standard library unless there's a reason to branch out. In this case, using pytest means that we don't have to perform conditional imports. Great! But the downside of using pytest is that we're forcing our readers to learn the ins and outs of yet another library, instead of allowing our readers to use their knowledge of tools from the standard lib.

I will also admit to a strong personal distaste for the "no API" design goal of pytest. One of the stand-out features of Python over other languages is that you can explicitly list what is being imported right at the top of the module. This makes Python much more readable than many other languages. pytest throws away this advantage.

I've never used tox, and if it provides some compelling advantage that cannot be acheived with standard lib tools, then that's worth taking in to consideration. Can you provide a stack trace or similar?

@omaciel
Copy link
Owner

omaciel commented Feb 23, 2015

@Ichimonji10 My personal preference is that we stick with tools from the standard library unless there's a reason to branch out.

Agreed. I want to make sure that we keep external (to the standard library) dependencies to a minimal.

Now, I wish I had a 2.6 system laying around... :/

@jlaska
Copy link
Contributor Author

jlaska commented Feb 24, 2015

Fair enough. PR updated for unittest2 support.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 074981e on jlaska:python26-support into * on omaciel:master*.

@@ -532,7 +532,7 @@ def gen_ipaddr(ip3=False, ipv6=False):

if ipv6:
# StackOverflow.com questions: generate-random-ipv6-address
ipaddr = u':'.join('{:x}'.format(
ipaddr = u':'.join('{0:x}'.format(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!!

@omaciel
Copy link
Owner

omaciel commented Feb 24, 2015

@jlaska other than my still pending doubt about being able to call sys.version_info.major from a 2.6 environment, and a question about the additional requirements file, everything looks good but we still need to handle the this failure. Do we need to update .travis.yaml to take into consideration how lintmay behave under 2.6???

@omaciel
Copy link
Owner

omaciel commented Feb 24, 2015

Ahhh I missed the comment about the bug with pylint and python 2.6. Guess one way to work around it would be to follow the same .travis.yaml and tox configurations shown here?

@jlaska
Copy link
Contributor Author

jlaska commented Feb 24, 2015

Ahhh I missed the comment about the bug with pylint and python 2.6. Guess one way to work around it would be to follow the same .travis.yaml and tox configurations shown here?

Yeah. My preferred approach is to have Travis use tox, so it mimics how one would test locally. I'll have that up for review shortly.

@jlaska jlaska force-pushed the python26-support branch 2 times, most recently from c76f44d to 92edf38 Compare February 24, 2015 13:46

class TestHTML(unittest.TestCase):
"""Test HTML generator."""
# support for py2.6
# matcher = re.compile('^<.*?>(.*?)</.*>$')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. Can it be clarified or dropped?

@jlaska
Copy link
Contributor Author

jlaska commented Feb 24, 2015

@Ichimonji10 ah, good catch. That should not have landed in the PR. Removed and updated PR.

- 3.3
- 3.4
env:
- TOXENV=py26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment above the list of environments noting that this is a duplicate of what is in tox.ini? Something like this:

env:
  # Should be identical to envlist in tox.ini. See:
  # http://tox.readthedocs.org/en/latest/config.html#confval-envlist=CSV
  - TOXENV=py26
  - TOXENV=py27
  - TOXENV=py33
  - TOXENV=py34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ichimonji10 Sure! It's easier to just have travis just run tox. However, if you want travis to run a job for each supported python version, we'll need to duplicate the TOXENV in .travis.yml. I presume that's how you'll want the results, since it's consistent with how tests were previously run.

I'll update the .travis.yml per your feedback.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like being able to view a matrix of results in Travis. There are times when a bug will only manifest itself in certain Python versions, and making that information easily discoverable is immensely valuable. That's why I'm OK with duplicating this information. If there is some method by which we can de-duplicate this information and still get a matrix of easily discoverable results, then that would be even cooler. But I don't know how to do that. I just don't know enough about the interactions between Travis and tox.

@Ichimonji10
Copy link
Contributor

ACK pending comment and commits being squashed.

* Adds a tox.ini to facilitate testing supported py_versions
* Add a requirements-optional-26.txt file
* Update fauxfactory/__init__.py to support 2.6 syntax
* Updates tests/test_*.py to conditionally import unittest2
* Update travis to run tests using tox
@jlaska
Copy link
Contributor Author

jlaska commented Feb 24, 2015

Commits squashed and incorporated feedback from @Ichimonji10

@omaciel
Copy link
Owner

omaciel commented Feb 24, 2015

ACK and thank you!!!

omaciel added a commit that referenced this pull request Feb 24, 2015
@omaciel omaciel merged commit baa8c30 into omaciel:master Feb 24, 2015
@omaciel
Copy link
Owner

omaciel commented Feb 24, 2015

Will release new version later today

@jlaska jlaska deleted the python26-support branch February 24, 2015 17:04
@jlaska
Copy link
Contributor Author

jlaska commented Feb 24, 2015

Yay, thanks @omaciel and @Ichimonji10

@Ichimonji10
Copy link
Contributor

Thank you, @jlaska. This is a definite improvement.

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

Successfully merging this pull request may close these issues.

None yet

4 participants