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

Correct handling of walrus operator in function args #417

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

thatch
Copy link
Contributor

@thatch thatch commented Nov 11, 2020

Summary

Previous behavior treated it as identical to equal, making a kwarg; it should
instead be a positional arg. Includes several tests to make sure that
whitespace handling is correct.

Fixes #416

Test Plan

Includes new tests that were failing before. I had some trouble ensuring the whitespace ended up in the right spot during development, so this includes a few probably unnecessary tests out of a general distrust that I did it right.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 11, 2020
@amyreese
Copy link
Member

Looks good to me, and I verified locally that it resolves the issue from usort.

Before fix:

(.venv) jreese@garbodor ~/workspace/usort main± » cat foo.py
foo.func(value := "some string literal")

(.venv) jreese@garbodor ~/workspace/usort main± » usort diff foo.py
--- a/foo.py
+++ b/foo.py
@@ -1,2 +1,2 @@
-foo.func(value := "some string literal")
+foo.func(value = "some string literal")

With fix:

(.venv) jreese@garbodor ~/workspace/usort main± » pip install -U git+https://github.com/thatch/libcst@issue416
Collecting git+https://github.com/thatch/libcst@issue416
  Cloning https://github.com/thatch/libcst (to revision issue416) to /private/var/folders/_b/6gpzshq548d4d_4bk1m5sq280000gn/T/pip-req-build-n3b3irzn
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Requirement already satisfied, skipping upgrade: typing-inspect>=0.4.0 in ./.venv/lib/python3.8/site-packages (from libcst==0.3.13) (0.6.0)
Requirement already satisfied, skipping upgrade: typing-extensions>=3.7.4.2 in ./.venv/lib/python3.8/site-packages (from libcst==0.3.13) (3.7.4.3)
Requirement already satisfied, skipping upgrade: pyyaml>=5.2 in ./.venv/lib/python3.8/site-packages (from libcst==0.3.13) (5.3.1)
Requirement already satisfied, skipping upgrade: mypy-extensions>=0.3.0 in ./.venv/lib/python3.8/site-packages (from typing-inspect>=0.4.0->libcst==0.3.13) (0.4.3)
Building wheels for collected packages: libcst
  Building wheel for libcst (PEP 517) ... done
  Created wheel for libcst: filename=libcst-0.3.13-py3-none-any.whl size=502635 sha256=9dd3fcfe16688992f2ff6bb0ddd8235f15fb595134768bbdb9aeb5660fcee259
  Stored in directory: /private/var/folders/_b/6gpzshq548d4d_4bk1m5sq280000gn/T/pip-ephem-wheel-cache-i73lgx32/wheels/cc/af/27/cece6ddd4a6a62f85a0ea3964b00c4fd3d176216786b578d03
Successfully built libcst
Installing collected packages: libcst
  Attempting uninstall: libcst
    Found existing installation: libcst 0.3.13
    Uninstalling libcst-0.3.13:
      Successfully uninstalled libcst-0.3.13
Successfully installed libcst-0.3.13
WARNING: You are using pip version 20.2.1; however, version 20.2.4 is available.
You should consider upgrading via the '/Users/jreese/workspace/usort/.venv/bin/python -m pip install --upgrade pip' command.

(.venv) jreese@garbodor ~/workspace/usort main± » usort diff foo.py

Copy link
Contributor

@jimmylai jimmylai left a comment

Choose a reason for hiding this comment

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

Other than the comment, LGTM.

# "key := value" assignment; positional
if equal.string == ":=":
val = convert_namedexpr_test(config, children)
assert isinstance(val, WithLeadingWhitespace)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use assert since it's noop in optimized mode. Let's just use if condition and raise when it returns unexpected result.

Suggested change
assert isinstance(val, WithLeadingWhitespace)
if isinstance(val, WithLeadingWhitespace):
return Arg(value=val.value)
else:
raise Exception("...")

Previous behavior treated it as identical to equal, making a kwarg; it should
instead be a positional arg.  Includes several tests to make sure that
whitespace handling is correct.

Fixes Instagram#416
@jimmylai jimmylai merged commit 31bae01 into Instagram:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Function call with walrus loses colon in roundtrip
4 participants