Skip to content

Remove support and tests for Python 2.7#234

Merged
jklukas merged 8 commits into
mainfrom
remove-2.7
Nov 2, 2021
Merged

Remove support and tests for Python 2.7#234
jklukas merged 8 commits into
mainfrom
remove-2.7

Conversation

@jklukas

@jklukas jklukas commented Oct 7, 2021

Copy link
Copy Markdown
Member

The next build will be 3.4+.

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

@jklukas jklukas requested a review from graingert October 7, 2021 23:06
Comment thread .travis.yml Outdated
- "3.6"
- "3.7"
- "3.8"
- "3.4"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No tests on 3.6-3.8?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think we've ever found an issue that's isolated to one of these intermediate versions. Given that tests are taking longer, it seems wasteful to consume resources on running tests across 5 different python versions. It seems sufficient to me to test on the oldest supported version and the newest version. What do you think?

@jklukas

jklukas commented Oct 8, 2021

Copy link
Copy Markdown
Member Author

I started messing with the tox configuration here and will need to do some more playing with it to get it working.

@Brooke-white Brooke-white left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given this change, would we still need compat.py?

@jklukas

jklukas commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

Given this change, would we still need compat.py?

Good catch. Will remove.

Comment thread .travis.yml
Comment on lines +16 to +17
- python: "3.9"
env: TOXENV=docs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A docs task was defined in tox.ini, but wasn't defined in the travis config. This allows it to run regularly in CI.

Comment thread README.rst

This dialect requires psycopg2 library to work properly. It does not provide
it as required, but relies on you to select the psycopg2 distribution you need:
This dialect requires either ``redshift_connector`` or ``psycopg2``

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unrelated docs fixup. I noticed this while restoring the docs task.

Comment thread docs/index.rst
ddl-compiler
dialect
commands
ddl

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was an error in the index; there's no rst file of this name.

Comment thread tox.ini
requests==2.7.0

[testenv:py{36,37,38,39}-sa{13,14}]
[testenv:py39-sa14]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To reduce test runtime, I've changed the py39 test to only include sa14, and the 35 test includes only sa13. Tests can complete now in under 15 minutes.

Comment thread tox.ini
-rrequirements-docs.txt
commands=
sphinx-build -W -b html -d {envtmpdir}/doctrees . {envtmpdir}/html
sphinx-build -b html -d {envtmpdir}/doctrees . {envtmpdir}/html

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed the -W flag because docs generation currently fails due to warnings. I filed #239 to follow up on addressing the warnings.

@jklukas jklukas merged commit c830f7b into main Nov 2, 2021
@jklukas jklukas deleted the remove-2.7 branch November 2, 2021 14:39
@jklukas jklukas removed the request for review from graingert November 2, 2021 14:40
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.

3 participants