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

Preview style: Don't collapse dummy implementation with comment #4063

Closed
JelleZijlstra opened this issue Nov 21, 2023 · 3 comments · Fixed by #4066
Closed

Preview style: Don't collapse dummy implementation with comment #4063

JelleZijlstra opened this issue Nov 21, 2023 · 3 comments · Fixed by #4066
Labels
C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. T: bug Something isn't working

Comments

@JelleZijlstra
Copy link
Collaborator

I found this change in our test suite while trying to stabilize the preview style:

 match command.split():
     case ["north"] | ["go", "north"]:
         current_room = current_room.neighbor("north")
-    case ["get", obj] | ["pick", "up", obj] | ["pick", obj, "up"]:
-        ...  # Code for picking up the given object
+    case (
+        ["get", obj] | ["pick", "up", obj] | ["pick", obj, "up"]
+    ): ...  # Code for picking up the given object

This is from the dummy_implementation preview change, #3796.

I think the new code looks worse here; we shouldn't use the dummy_implementation branch if there is a comment after the ellipsis.

@JelleZijlstra JelleZijlstra added T: bug Something isn't working C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. labels Nov 21, 2023
JelleZijlstra added a commit to JelleZijlstra/black that referenced this issue Nov 21, 2023
@JelleZijlstra
Copy link
Collaborator Author

I tried to change this but it causes some problems in typeshed:

-    def __mul__(cls: type[_CT], other: int) -> type[Array[_CT]]: ...  # type: ignore[misc]  # pyright: ignore[reportGeneralTypeIssues]
-    def __rmul__(cls: type[_CT], other: int) -> type[Array[_CT]]: ...  # type: ignore[misc]  # pyright: ignore[reportGeneralTypeIssues]
+    def __mul__(cls: type[_CT], other: int) -> type[Array[_CT]]:
+        ...  # type: ignore[misc]  # pyright: ignore[reportGeneralTypeIssues]
+
+    def __rmul__(cls: type[_CT], other: int) -> type[Array[_CT]]:
+        ...  # type: ignore[misc]  # pyright: ignore[reportGeneralTypeIssues]

Will have to think more about the right approach here.

Also considering to turn off the dummy_implementations change for suites that aren't functions or classes.

@hauntsaninja
Copy link
Collaborator

Yeah, let's avoid changing the logic with comments. Only doing dummy impls for classes and functions seems like a good way to fix the issue.

@JelleZijlstra
Copy link
Collaborator Author

Sounds good, I updated the PR to apply the dummy impl logic only to classes and functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: preview style Issues with the preview and unstable style. Add the name of the responsible feature in the title. T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants