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

ylib2to3 -- Fork lib2to3 with some blib2to3 backports; add structured pattern matching (match) and parenthesized-context-managers statement support #1067

Merged
merged 103 commits into from
Jun 9, 2023

Conversation

Spitfire1900
Copy link
Contributor

@Spitfire1900 Spitfire1900 commented Apr 8, 2023

closes #983, #1045, #1085 (match)

Opened after @char101 said it was okay to open a PR for this. Many thanks to them for adding match statement support for yapf!

lib2to3 is no longer maintained, this forks lib2to3 and includes some modifications backported from black's blib2to3.

I've looked into modifying yapf to work directly with blib2to3 instead of forking it but this work is in-progress and may not be possible. In addition it holds the risk that the black team does not consider blib2to3 as meant for other projects to consume.

I personally feel that a better long term approach would be for the black and yapf projects to externalize blib2to3 as a separate package that each project depends on rather than having each maintain their own fork.

@google-cla
Copy link

google-cla bot commented Apr 8, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Spitfire1900
Copy link
Contributor Author

PR is for #1045

@thiagowfx
Copy link

Thanks for your PR, Kyle! Is it feasible to split it into smaller ones? It's quite hard to review it as it is.

Disclaimer: I am not a maintainer of this repo and not familiar with the yapf implementation, so I cannot guarantee this will be merged, even if it's split into smaller CLs. However, where they are easy to review and non-controversial w.r.t. technicalities, I am happy to merge them.
It would be better if one of the core members reviewed it though, given that it's non-trivial (see log history to find them).

@char101
Copy link
Contributor

char101 commented Apr 10, 2023

Although there are a lot of changes, the change to yapf itself is actually trivial. Here is what I did:

  1. Copy lib2to3 from python 3.10 to yapf/yalib2to3
  2. Manually apply the changes from the commits of https://github.com/psf/black/commits/main/src/blib2to3 which are not black specific
  3. Change the imports from lib2to3 to ylib2to3
  4. Add match and case block node to yapf

@Spitfire1900
Copy link
Contributor Author

We'll still need to fix some GitHub action errors and pytest failures before merging.

Meanwhile I'll get the CLA signed.

README.rst Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@thiagowfx
Copy link

Could you add a few tests?

@bwendling
Copy link
Member

Thank you for this work! I need to consult internally to make sure we can allow this code into the tree.

In the meantime, there are some failures in the checks. Could you look into them?

@Spitfire1900
Copy link
Contributor Author

I am currently at 8 failures with pytest on 3.10

================================================================================================================================================= short test summary info =================================================================================================================================================
FAILED yapftests/reformatter_pep8_test.py::TestsForPEP8Style::testHangingIndentCollision - AssertionError: Code format mismatch:
FAILED yapftests/reformatter_python3_test.py::TestsForPython3Code::testContinuationIndentWithAsync - AssertionError: Code format mismatch:
FAILED yapftests/yapf_test.py::CommandLineTest::testReformattingSkippingLines - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[249 chars]le\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[253 chars]le\n"
FAILED yapftests/yapf_test.py::CommandLineTest::testReformattingSkippingSingleLine - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[233 chars]ss\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[237 chars]ss\n"
FAILED yapftests/yapf_test.py::CommandLineTest::testReformattingSkippingToEndOfFile - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[461 chars]ss\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[465 chars]ss\n"
FAILED yapftests/yapf_test.py::CommandLineTest::testReformattingSpecificLines - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[216 chars]ss\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[220 chars]ss\n"
FAILED yapftests/yapf_test.py::CommandLineTest::testRetainingHorizontalWhitespace - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[250 chars]ss\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[254 chars]ss\n"
FAILED yapftests/yapf_test.py::CommandLineTest::testRetainingVerticalWhitespace - AssertionError: "def [65 chars]aaaa'\n        and xxxxxxxxxxxx.yyyyyyyy(zzzzz[220 chars]ss\n" != "def [65 chars]aaaa' and\n            xxxxxxxxxxxx.yyyyyyyy(z[224 chars]ss\n"
============================================================================================================================================= 8 failed, 591 passed in 21.78s ==============================================================================================================================================

@Spitfire1900
Copy link
Contributor Author

@char101 it looks like there is favorability with eventually merging in this PR but there are some failing tests. Resolving these are a little bit out of my wheelhouse, would you be interested in submitting changes to get the above mentioned tests passing?

@Spitfire1900
Copy link
Contributor Author

Spitfire1900 commented May 26, 2023

I've implemented the following requirements

  1. Have a third_party directory at the root of the git repo
  2. Do not install a site-packages/third_party directory at install time.

This:

  1. Moves yapf/third_party content to third_party/yapf_third_party in the repo.
  2. Installs yapf_third_party into site-packages/yapf_third_party at install time.

@Spitfire1900
Copy link
Contributor Author

Spitfire1900 commented May 27, 2023

Alright, requested changes made and merged in upstream.

@Spitfire1900
Copy link
Contributor Author

Finished up some GH workflow work as installing the project prior to running pytest is now required as dependencies importlib-metadata and platformdirs have been added.

@Spitfire1900
Copy link
Contributor Author

@bwendling this is ready for re-review.

@Spitfire1900
Copy link
Contributor Author

CC'ing @bwendling and @thiagowfx for review.

@Spitfire1900 Spitfire1900 requested a review from thiagowfx June 7, 2023 18:42
@Spitfire1900
Copy link
Contributor Author

What is preferred for site-packages layout for each of the third party modules? Should _ylib2to3 or yapf_diff be at the root of site-packages? Only yapf_diff? Neither?

This is current:

site-packages
\_ yapf_third_party
   \_ _ylib2to3
   \_ yapf_diff
\_ yapf-0.33.0.dist-info
\_ yapf
\_ yapftests

I think that this is probably a better fit, or at least that yapf_diff should exist at the root of site-packages

site-packages
\_ _ylib2to3
\_ yapf_diff
\_ yapf-0.33.0.dist-info
\_ yapf
\_ yapftests

@bwendling
Copy link
Member

What is preferred for site-packages layout for each of the third party modules? Should _ylib2to3 or yapf_diff be at the root of site-packages? Only yapf_diff? Neither?

I don't have much of an opinion on this, but I think we should do that in a separate PR to not jar things too much.

Apologies for the lateness of this review. I'm dealing with emergency issues right now, so work has been spotty.

@bwendling
Copy link
Member

Thank you for taking this on! This is a major step forward in the life of YAPF.

@bwendling bwendling merged commit 7c408b9 into google:main Jun 9, 2023
@Spitfire1900
Copy link
Contributor Author

@bwendling Thank you very much, may this be released to PyPi?

@thinkwelltwd
Copy link

@Spitfire1900, I've watched this PR with bated breath throughout its long development. Emoji reactions just aren't adequate to communicate my thanks and appreciation for your work.

And thank you, @bwendling, for yapf! It's way and above the best python formatter, as far as I'm concerned.

@Spitfire1900
Copy link
Contributor Author

Most of the credit goes to @char101 for the adding the necessary logic to lib2to3 to support new language features. My work was primarily prepping it for merge into upstream. ☺️

@bwendling
Copy link
Member

I concur with @thinkwelltwd :-)

This probably deserves its own PyPi release. I'll do one soon.

Again, thank you for all of your work, @Spitfire1900!

@bwendling
Copy link
Member

And thanks to @char101 for your work! This pushes YAPF past a major hurdle.

@hartwork
Copy link
Contributor

CC #1110

@eli-schwartz
Copy link

Fixed by using using https://pypi.org/project/importlib-metadata

I would always recommend you do a try/except and see if you can first use https://docs.python.org/3/library/importlib.metadata.html

This reduces the dependencies and makes it faster to install. It is available in the stdlib for nearly every supported python version. :)

@Spitfire1900
Copy link
Contributor Author

@eli-schwartz , this is a recommendation that has been advised as part of the tomllib PR: #1152

@Spitfire1900
Copy link
Contributor Author

Third party dependencies added by this PR are removed for newer python versions with #1158

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.

match case statement raise lib2to3.pgen2.parse.ParseError: bad input
8 participants