Comments

Use ruff for linting#771

Open

dandavison wants to merge 16 commits intomain from

lint

Open

Use ruff for linting #771
dandavison wants to merge 16 commits intomain from
lint

Conversation

@dandavison

Copy link

Contributor

@dandavison dandavison commented Feb 18, 2025

WISOTT

PR targets maturin branch #768

@dandavison dandavison requested a review from a team as a code owner

February 18, 2025 15:48

cretz

cretz reviewed

Feb 18, 2025

View reviewed changes

tests/worker/workflow_sandbox/test_importer.py Outdated

with pytest.raises(RestrictedWorkflowAccessError) as err:

with Importer(restrictions, RestrictionContext()).applied():

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

@dandavison dandavison Feb 21, 2025

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.

temporalio/api/cloud/cloudservice/v1/__init__.py Outdated

# 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

@dandavison dandavison Feb 21, 2025

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)

temporalio/worker/workflow_sandbox/_restrictions.py

Comment on lines +936 to +937

# TODO: wat

f(self, other) # type: ignore # noqa: F821

Copy link

Member

@cretz cretz Feb 18, 2025

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

@dandavison dandavison Feb 21, 2025

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

dandavison added 9 commits

February 21, 2025 08:58

@dandavison


 Manual lint fixes

23892f3

@dandavison


 ignore pb2 grpc

ccd473b

@dandavison


 tweak

@dandavison


 uv run ruff check --fix

@dandavison


 Revert over-zealous ruff changes

413cf4c

@dandavison


 Add explanation

89158ee

@dandavison

WIP

5797e99

@dandavison


 Fix overzealous ruff check --fix

2ba340c

@dandavison


 Get rid of type: ignores

67ee4bb

@dandavison dandavison force-pushed the lint branch from a4803fc to 67ee4bb Compare

February 21, 2025 14:21

dandavison

dandavison commented

Feb 21, 2025

View reviewed changes

tests/test_converter.py

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

@dandavison dandavison Feb 21, 2025

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

@cretz cretz Feb 21, 2025

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

dandavison commented Feb 21, 2025

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

cretz

cretz approved these changes

Feb 21, 2025

View reviewed changes

@dandavison


 Use features main branch

051b718

@dandavison dandavison force-pushed the lint branch from 2112710 to 051b718 Compare

February 21, 2025 17:38

@dandavison dandavison force-pushed the lint branch 3 times, most recently from 87eb719 to 3f1a956 Compare

March 1, 2025 15:10

@dandavison


 Revert

b711b4f

@dandavison dandavison force-pushed the lint branch from 3f1a956 to b711b4f Compare

March 1, 2025 19:42

@dandavison


 The server now uses UUID v7 for runID

831fec2

dandavison

dandavison commented

Mar 1, 2025

View reviewed changes

tests/worker/test_workflow.py

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

@dandavison dandavison Mar 1, 2025

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

@dandavison


 Use my server

bf216f8

@dandavison dandavison force-pushed the lint branch from 50ff645 to bf216f8 Compare

March 2, 2025 17:09

dandavison added 3 commits

March 8, 2025 09:11

@dandavison


 Revert "Use my server"

41199e6

This reverts commit bf216f8.

@dandavison


 Rearrange

510bc13

@dandavison


 Change to BaseException to preserve original semantics

5c0eaaa

Labels

None yet

2 participants

@dandavison @cretz

Comments

Conversation

@dandavison dandavison commented Feb 18, 2025

Uh oh!

@cretz cretz Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

@dandavison dandavison Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

@cretz cretz Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

@dandavison dandavison Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

@cretz cretz Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

@dandavison dandavison Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

@dandavison dandavison Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

@cretz cretz Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

dandavison commented Feb 21, 2025

Uh oh!

@dandavison dandavison Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants