-
Notifications
You must be signed in to change notification settings - Fork 242
Ruff fixes #786
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
Ruff fixes #786
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM.
Some OPTIONAL microoptimizations for your consideration.
astral-sh/ruff#20122 Was merged into the just released ˋruff` v0.12.12. |
7a8fc35
to
2903da1
Compare
@@ -24,7 +24,7 @@ async def run(): | |||
drone = System() | |||
await drone.connect(system_address="udpin://0.0.0.0:14540") | |||
|
|||
tasks = [ | |||
_tasks = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unusual to assemble these tasks but not await them. Are we certain that they will be run at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, so this code was working fine but ruff complained that the tasks were dangling: https://docs.astral.sh/ruff/rules/asyncio-dangling-task/.
So, the advice was to create a strong reference to the tasks. However, now ruff complains that tasks are assigned but never used. It feels like you can't win.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cclauss ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cclauss you need to actually do a read&write conversation, not just write only please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong... This usage is correct.
>>> import asyncio
>>> async def print_char(char: str) -> None:
... print(char)
...
>>> async def run(chars: str) -> None:
... _tasks = [asyncio.create_task(print_char(char)) for char in chars]
...
>>> asyncio.run(run("abcdefg"))
a
b
c
d
e
f
g
.github/workflows/main.yml
Outdated
@@ -27,7 +27,7 @@ jobs: | |||
- name: Check style | |||
run: | | |||
pipx run pycodestyle examples/* | |||
pipx run ruff check --select=ASYNC,RUF006 | |||
pipx run ruff check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipx run ruff check | |
pipx run ruff check # default rules | |
pipx run ruff check --select=ASYNC,RUF006 # Special rules for asyncio |
This changes things. Normally, ASYNC
and RUF
are off by default, so this change removes those rules and adds the default rules. https://docs.astral.sh/ruff/rules Let's run both just to be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Thanks.
_tasks = [ | ||
asyncio.create_task(observe_current_settings(drone)), | ||
asyncio.create_task(observe_camera_mode(drone)), | ||
asyncio.create_task(observe_possible_setting_options(drone)), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax would delete tasks as they are completed, as discussed in ruff rule RUF006
.
_tasks = [ | |
asyncio.create_task(observe_current_settings(drone)), | |
asyncio.create_task(observe_camera_mode(drone)), | |
asyncio.create_task(observe_possible_setting_options(drone)), | |
] | |
tasks = { | |
asyncio.create_task(observe_current_settings(drone)), | |
asyncio.create_task(observe_camera_mode(drone)), | |
asyncio.create_task(observe_possible_setting_options(drone)), | |
} | |
for task in tasks: | |
task.add_done_callback(tasks.discard) |
Co-authored-by: Christian Clauss <[email protected]>
Co-authored-by: Christian Clauss <[email protected]>
Co-authored-by: Christian Clauss <[email protected]>
e002b4a
to
c428076
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's an improvement, let's get it in before I have to fix more conflicts.
Closes #771.
@cclauss