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

[BEAM-3738] Enable py3 lint #4801

Closed
wants to merge 2 commits into from
Closed

[BEAM-3738] Enable py3 lint #4801

wants to merge 2 commits into from

Conversation

aaltay
Copy link
Member

@aaltay aaltay commented Mar 6, 2018

Since we verified that Jenkins workers have python3 installed (https://issues.apache.org/jira/browse/BEAM-3671) we should be able to enable py3 lint.

R: @alanmyrvold

@aaltay
Copy link
Member Author

aaltay commented Mar 6, 2018

R: @holdenk

@cclauss
Copy link

cclauss commented Mar 7, 2018

What version of Python 3 are we testing under? The other test blocks do python --version and pip --version but the lint_py3 one does not.

@aaltay
Copy link
Member Author

aaltay commented Mar 7, 2018

@cclauss Good catch. I believe we have 3.4.3 version based on (https://issues.apache.org/jira/browse/BEAM-3671). I will update tox.ini to print version for this use too.

@aaltay
Copy link
Member Author

aaltay commented Mar 7, 2018

Updated, but I think we need to think about one more case. It turned out that printing the version was a good idea, because tox was defaulting to python2. Running it with python3 uncovered an issue: setup.py has python_requires='>=2.7,<3.0', which makes tox fail. We can remove that but we are not py3 compatible yet.

@cclauss
Copy link

cclauss commented Mar 7, 2018

We could reduce our exposure by making it python_requires='>=2.7,<=3.4' because 95% of users will no longer be using Python 3.4 when the current Python is 3.6.4 and 3.7 is in beta.

Or reduce our exposure even more by making it python_requires='>=2.7,==3.4.3' because that is the exact Python 3 version that we have and the current Python 3.4 is version 3.4.7.

@holdenk
Copy link
Contributor

holdenk commented Mar 7, 2018

So @cclauss I don't think that limiting to 3.4.3 is great since someone might read it and then just try to use 3.4.3. Personally I'd use an env variable to (e.g. LINTING) and in linting have a different requirement. We also probably want folks to be able to run tox on their own machines since its in the guide.

But I like the direction of selectively unblocking setup.py for testing/linting so that we more easily track our progress and avoid regressions.

@holdenk
Copy link
Contributor

holdenk commented Mar 13, 2018

@aaltay what are your thoughts?

@aaltay
Copy link
Member Author

aaltay commented Mar 14, 2018

I have not had a chance to work on this after the my previous comment. I like the idea of using an environment variable.

@udim
Copy link
Member

udim commented Mar 16, 2018

I've created #4877, which should include the changes here as well.

@aaltay
Copy link
Member Author

aaltay commented Mar 17, 2018

Thank you @udim. Closing this PR in favor of #4877.

@aaltay aaltay closed this Mar 17, 2018
@robertwb
Copy link
Contributor

robertwb commented Mar 17, 2018 via email

@aaltay aaltay deleted the lint3 branch December 20, 2022 16:53
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.

5 participants