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

Adds ruff, a drop-in replacement for black #423

Closed
wants to merge 2 commits into from
Closed

Conversation

rachfop
Copy link
Contributor

@rachfop rachfop commented Nov 9, 2023

What was changed

Added isort: skip_file in the .gitignore file, because ruff will follow isorts commands.
Replaced black with ruff in the .pyproject.toml` file.

  • ran poe format && poe lint

There were a lot of changes made.

  • removed unused imports
  • removed/added white space according to pydoc style
    If this is too much, let me know and I close this PR and find a way later to make less changes.
    Looking for a quick win.

Why?

#421

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@rachfop rachfop requested a review from a team as a code owner November 9, 2023 17:45
@cretz
Copy link
Member

cretz commented Nov 9, 2023

Can you work out the CI kinks? Make sure the app lints and tests all complete properly. Can mark as draft while you work through them.

@rachfop rachfop marked this pull request as draft November 9, 2023 17:54
@@ -41,7 +41,7 @@ types-protobuf = ">=3.20"
typing-extensions = "^4.2.0"

[tool.poetry.dev-dependencies]
black = "^23.1.0"
ruff = "^0.1.5"
cibuildwheel = "^2.11.0"
grpcio-tools = "^1.48.0"
isort = "^5.11.5"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isort = "^5.11.5"

@@ -1,3 +1,4 @@
# isort: skip_file
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# isort: skip_file

I don't think this is needed

Copy link
Member

Choose a reason for hiding this comment

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

I am worried a lot of things changed in here inadvertently and am also worried that by not using an older version of Poetry we may be forcing users to upgrade (which has its own issues at the moment)

Comment on lines -16 to -29
from google.protobuf import timestamp_pb2 as google_dot_protobuf_dot_timestamp__pb2

from temporalio.api.common.v1 import (
message_pb2 as temporal_dot_api_dot_common_dot_v1_dot_message__pb2,
)
from temporalio.api.dependencies.gogoproto import (
gogo_pb2 as dependencies_dot_gogoproto_dot_gogo__pb2,
)
from temporalio.api.enums.v1 import (
batch_operation_pb2 as temporal_dot_api_dot_enums_dot_v1_dot_batch__operation__pb2,
)
from temporalio.api.enums.v1 import (
reset_pb2 as temporal_dot_api_dot_enums_dot_v1_dot_reset__pb2,
)
Copy link
Member

Choose a reason for hiding this comment

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

These imports are intentionally side effects and should not be removed. I wonder if we need to make ruff ignore the entire temporalio/api folder

@@ -13,7 +13,6 @@

import temporalio.bridge.runtime
import temporalio.bridge.temporal_sdk_bridge
from temporalio.bridge.temporal_sdk_bridge import RPCError
Copy link
Member

Choose a reason for hiding this comment

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

This is imported for side effecting reasons as are some others in some other files. Feel free to find ways to workaround.

@dandavison
Copy link
Contributor

Thanks @rachfop! We followed through with this in #566

@dandavison dandavison closed this Jul 23, 2024
@cretz cretz deleted the adds-ruff-formatter branch July 23, 2024 14:14
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.

3 participants