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

Do not match keywords named 'match' #1111

Merged
merged 11 commits into from
Sep 4, 2023
Merged

Do not match keywords named 'match' #1111

merged 11 commits into from
Sep 4, 2023

Conversation

Spitfire1900
Copy link
Contributor

@Spitfire1900 Spitfire1900 commented Jun 17, 2023

Closes #1110 , do not match variable names named 'match'.

Signed-off-by: Kyle Gottfried <[email protected]>
@Spitfire1900
Copy link
Contributor Author

Spitfire1900 commented Jun 17, 2023

I've verified locally that the diff is clean.

:~/Repos/yapf/wt/1110$ curl https://raw.githubusercontent.com/hartwork/wnpp.debian.net/master/wnpp_debian_net/management/commands/importdebbugs.py > importdebbugs.py \
&& curl https://raw.githubusercontent.com/hartwork/wnpp.debian.net/master/.style.yapf > ..style.yapf \
&& pipx run --spec=. yapf --style ..style.yapf --diff importdebbugs.py
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 15430  100 15430    0     0  40814      0 --:--:-- --:--:-- --:--:-- 40820
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   126  100   126    0     0    989      0 --:--:-- --:--:-- --:--:--   992
⚠️  yapf is already on your PATH and installed at /home/kyle/.local/bin/yapf. Downloading and running anyway.
kyle@mainstay:~/Repos/yapf/wt/1110$

@hartwork , can you write a test for this case?

@hartwork
Copy link
Contributor

@Spitfire1900 did you see hartwork/wnpp.debian.net@0a49666 ?

@Spitfire1900
Copy link
Contributor Author

Spitfire1900 commented Jun 18, 2023

@Spitfire1900 did you see hartwork/wnpp.debian.net@0a49666 ?

I'll try adding it later, @ mentioned yourself because I wasn't going to be available until tomorrow to complete this PR.

@char101
Copy link
Contributor

char101 commented Jun 18, 2023

Minimal test case

match = (
    1,
    2,
)

Without the fix

..\env\scripts\python -m yapf -d ..\test.py
--- ..\test.py  (original)
+++ ..\test.py  (reformatted)
@@ -1,4 +1,4 @@
 match = (
-    1,
-    2,
+        1,
+        2,
 )

@Spitfire1900
Copy link
Contributor Author

The sample test added with this commit passes on both tag v0.40.1 and this branch, at this time I am unsure why. The attached file generates the expected output below on v0.40.1 and no output on this branch.
matchtest.py.txt

 pipx run yapf==0.40.1 --no-local-style --diff matchtest.py 
⚠️  yapf is already on your PATH and installed at /home/kyle/Repos/yapf/wt/1110/.venv/bin/yapf. Downloading and running anyway.
--- matchtest.py        (original)
+++ matchtest.py        (reformatted)
@@ -4,11 +4,11 @@
 @staticmethod
 def _parse_wnpp_issue_subject(subject) -> tuple[str, str, str]:
     match = re.match(
-        '^(?:[Ss]ubject: )?(?P<kind>[A-Z]{1,3}): ?(?P<package>[^ ]+)(?:(?: --| -| —|:) (?P<description>.*))?$',
-        subject)
+            '^(?:[Ss]ubject: )?(?P<kind>[A-Z]{1,3}): ?(?P<package>[^ ]+)(?:(?: --| -| —|:) (?P<description>.*))?$',
+            subject)
 
 
 match = (
-    1,
-    2,
+        1,
+        2,
 )

@Spitfire1900
Copy link
Contributor Author

@bwendling do you have any input on why the manual test referenced in #1111 (comment) performs as expected but the pytest test does not?

@bwendling
Copy link
Member

@bwendling do you have any input on why the manual test referenced in #1111 (comment) performs as expected but the pytest test does not?

@Spitfire1900 I don't. Just checking. The diff in this comment is wrong and the behavior of matchtest.py is right?

@Spitfire1900
Copy link
Contributor Author

The diff in that comment is the current behavior of the main branch today, it is not expected.

With this PR there is no diff, which is expected.

However I am having difficult writing a validation test as the test added to reformatter_basic_test.py passes against both google:main and Spitfire1900:1110, which is not expected.

@bwendling
Copy link
Member

Ah! I see. The problem is probably in _GetNewlineColumn. My guess is that the code that would call _IsCompoundStmt isn't being executed for some reason or another. Maybe having to do with the style?

@Spitfire1900
Copy link
Contributor Author

So I was unable to get a test to perform as expected, so I am proposing this PR without a test case.

@Spitfire1900 Spitfire1900 marked this pull request as ready for review August 31, 2023 03:47
@Spitfire1900
Copy link
Contributor Author

@bwendling is it okay if this is merged without new tests?

@bwendling bwendling merged commit 77c29ef into google:main Sep 4, 2023
11 checks passed
@bwendling
Copy link
Member

Sure! Thanks. :-)

@Spitfire1900 Spitfire1900 deleted the 1110 branch September 4, 2023 12:15
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.

[0.40.0] Mistakes variable "match" for a match statement?
4 participants