search

web-platform-tests / results-collection

Other
41 stars 46 forks source link

Use print() function in both Python 2 and Python 3 #641

Open cclauss opened 5 years ago

cclauss commented 5 years ago

Pull Request Template

Thanks for the PR, you probably worked hard on it! Before you submit it for review, please make sure you read our guidelines for contributing to this repository.

Try to make the job easy for your reviewer! Below is a template you can use as a guide for what context your reviewer might need.
If you changed any dev procedures, consider also updating the README.

Description

Legacy print statements are syntax errors in Python 3 but print() function works as expected in both Python 2 and Python 3.

Review Information

Changes

Requirements

jugglinmike commented 5 years ago

Thanks for the patch, @cclauss. We don't currently support running this project in Python 3, so I'm reluctant to accept changes which suggest otherwise. While Python 3 is definitely a good goal, we currently lack the resources (both in terms of development effort and in terms of tooling) to ensure proper functionality there. Without that, we'll risk silently regressing with every future change. If you're interested in using the project in Python 3, I think the most responsible way to start is by extending our automated testing infrastructure.

It might not be worth the effort, though. That's because we're in the process of reimplementing the services this project provides in various third-party platforms (e.g. TaskCluster and Azure Pipelines). See gh-640, for example.

cclauss commented 5 years ago

Thank you for your detailed explanation. These changes will work in both Python 2 and Python 3 so there is no risk in accepting them now. See https://docs.python.org/2/library/functions.html#print Python 2 is no longer supported at the end of this year so most users of web-platform-tests will be wanting to run them on Python 3 or they will search for alternative solutions. https://pythonclock.org

cclauss commented 5 years ago

Travis CI now runs Python 3 tests in __allow_failures__ mode.

Also https://docs.python.org/3/library/unittest.html#deprecated-aliases

jugglinmike commented 5 years ago

These changes will work in both Python 2 and Python 3 so there is no risk in accepting them now. See https://docs.python.org/2/library/functions.html#print

The risk of silent regression comes from future changes, not the validity of this patch.

Python 2 is no longer supported at the end of this year so most users of web-platform-tests will be wanting to run them on Python 3 or they will search for alternative solutions. https://pythonclock.org

You are correct about Python 2's end-of-life, but this project is not intended for use by most users of web-platform-tests. The code here is used to fetch browsers and collect results on a regular interval. It uses the infrastructure which is maintained in the web-platform-tests project (largely within the tools directory). That's the code which is of interest to most users of web-platform-tests. While achieving Python 3 compatibility seems tractable for this project, it will be of limited use as long as WPT itself requires Python 2.

They say that if you're not part of the solution, then you're part of the problem. I'm willing to improve things here in advance of WPT, but we need to do that in a way where we can automatically verify correctness of future changes. It's great to see the tests running in CI--that's exactly what I had in mind. However, if it's failing and allowed to fail, then we haven't reached Python 3 compatibility, and we won't know when new commits move us farther from that goal.

And just to be clear: the reason I mentioned re-implementing results collection via third-party services is that once the work is complete, this project will be decommissioned. I'm happy to review and merge a maintainable solution in the mean time--I just want to be sure you're aware of that before putting more time in to this.