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

Use ruff for linting #771

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Use ruff for linting #771

wants to merge 16 commits into from

Conversation

dandavison
Copy link
Contributor

WISOTT

PR targets maturin branch #768

@dandavison dandavison requested a review from a team as a code owner February 18, 2025 15:48
import tests.worker.workflow_sandbox.testmodules.invalid_module
pass
Copy link
Member

Choose a reason for hiding this comment

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

Was this meant to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, no this was overzealous ruff check --fix autofixing.

@@ -194,9 +194,9 @@

# gRPC is optional
try:
import grpc
import grpc # noqa # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Just for my own curiosity, what was failing here that is requiring these two comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

temporalio/api/cloud/cloudservice/v1/__init__.py:197:12: F401 `grpc` imported but unused; consider using `importlib.util.find_spec` to test for availability

I've added the error code.

The type: ignore was to silence Pylance in VSCode. However, our pyright lint passes, so I've removed the type: ignore -- I'll add it back in if and when our lint requires it (and perhaps get my IDE type-checking in sync with our CI type-checking)

Comment on lines +936 to +937
# TODO: wat
f(self, other) # type: ignore # noqa: F821
Copy link
Member

Choose a reason for hiding this comment

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

I do not know what I was thinking here, but it's likely a bug. Would definitely welcome a test that covers this and fixes it to do what it is supposed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 #772

@dandavison dandavison force-pushed the maturin branch 6 times, most recently from e89f5b2 to bc54894 Compare February 19, 2025 02:40
Base automatically changed from maturin to main February 19, 2025 14:42
@@ -36,7 +36,6 @@
import temporalio.api.common.v1
import temporalio.common
from temporalio.api.common.v1 import Payload, Payloads
from temporalio.api.common.v1 import Payload as AnotherNameForPayload
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it was deliberate but also appears to have no purpose today.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

@dandavison dandavison requested a review from cretz February 21, 2025 14:35
@dandavison
Copy link
Contributor Author

Addressed comment and done another pass over the diff; this should be ready to merge.

@@ -222,7 +220,7 @@ async def test_workflow_info(client: Client, env: WorkflowEnvironment):
assert info["retry_policy"] == json.loads(
json.dumps(dataclasses.asdict(retry_policy), default=str)
)
assert uuid.UUID(info["run_id"]).version == 4
assert uuid.UUID(info["run_id"]).version == 7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The server switched to v7 UUIDs for runID at temporalio/temporal#6933

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.

2 participants