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

Support Python >= 3.6 #547

Merged
merged 11 commits into from May 27, 2020
Merged

Support Python >= 3.6 #547

merged 11 commits into from May 27, 2020

Conversation

naoyak
Copy link
Collaborator

@naoyak naoyak commented May 26, 2020

Description of changeset

Proposing dropping support for Python versions < 3.6 to reduce maintenance overhead and unlock some dependency upgrades (#300 (comment)).

Test Plan:

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@bulam
Copy link
Collaborator

bulam commented May 26, 2020

@naoyak Thanks for putting it in! I think this LGTM. @etr2460 Any thoughts?

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@naoyak naoyak force-pushed the update-ci branch 2 times, most recently from 5bb2631 to a4c2a26 Compare May 27, 2020 06:50
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@etr2460
Copy link
Collaborator

etr2460 commented May 27, 2020

@john-bodley or @serenajiang, would you mind taking a look at this?

@bulam
Copy link
Collaborator

bulam commented May 27, 2020

@naoyak I think there might be a lot of users using Python 2.7 on their own computers. If they try to add posts to the repo using the older version of the knowledge repo compatible with Python 2.7 do you think there could be compatability issues?

@naoyak
Copy link
Collaborator Author

naoyak commented May 27, 2020

@bulam personally I think it's reasonable to require Python 3 for those who want to use the latest version of KR, given that Python 2 has been EOL'd and most major libraries have dropped support. Perhaps it makes sense to ship this in a new major/minor release along with your styling revamp in #548?

Copy link
Collaborator

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I only had one small comment but overall this LGTM.

@@ -77,7 +77,7 @@ def show_views():

methods = ','.join(rule.methods)
url = url_for(rule.endpoint, **options)
line = urllib.unquote("{:50s} {:20s} {}".format(rule.endpoint, methods, url))
line = urllib.parse.unquote("{:50s} {:20s} {}".format(rule.endpoint, methods, url))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use the logic below in terms of importing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@john-bodley sorry can you clarify? Would it be better to from urllib.parse import unquote at the top of the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I see what you were referring to. Best to keep consistent 👍

@john-bodley
Copy link
Collaborator

@bulam I agree with @naoyak here. It's pretty easy for users to support multiple versions of Python locally via pyenv.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@naoyak naoyak merged commit f1aef9f into airbnb:master May 27, 2020
@naoyak naoyak mentioned this pull request May 28, 2020
john-bodley added a commit that referenced this pull request Nov 16, 2020
naoyak pushed a commit that referenced this pull request Nov 16, 2020
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

5 participants