Skip to content

added explicit typing to all async pipeline commands #8325

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jul 19, 2022
Merged

added explicit typing to all async pipeline commands #8325

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cda4f2d
added explicit typing to all async pipeline commands
bentheiii Jul 18, 2022
d3f66fe
added blank line
bentheiii Jul 18, 2022
78f4d2f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 18, 2022
bee478c
restored redis inheritance
bentheiii Jul 19, 2022
40ca3e0
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 19, 2022
35d4c69
fix
bentheiii Jul 19, 2022
63c3319
fixed the allowlist
bentheiii Jul 19, 2022
2a698a1
another fix
bentheiii Jul 19, 2022
0886d79
optionals are now incomplete
bentheiii Jul 19, 2022
a03eeb8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 19, 2022
307bce3
fix allowlist
bentheiii Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions stubs/redis/@tests/stubtest_allowlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ redis.typing.CommandsProtocol.__init__
# unclear problems
redis.Sentinel.master_for
redis.Sentinel.slave_for
redis.asyncio.client.Pipeline.command_info
redis.asyncio.client.Pipeline.debug_segfault
redis.asyncio.client.Pipeline.memory_doctor
redis.asyncio.client.Pipeline.memory_help
redis.asyncio.client.Pipeline.script_debug
redis.asyncio.client.Pipeline.shutdown
Comment on lines +20 to +25
Copy link
Member

@AlexWaygood AlexWaygood Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These allowlist entries are all because the methods in question are async def methods at runtime but not async def methods in the stub.

  1. Are you sure it's correct to have them as not being async def in the stub?
  2. If you are sure, can you put these allowlist entries in a separate section, and add a comment explaining why we're ignoring the stubtest errors? I don't think these entries belong in the "unclear problems" section, since we know why stubtest is complaining here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @AlexWaygood

I'm sure in the sense that it doesn't really matter, in principle this function should be syncronous in the pipeline, just like all other redis commands. In practice, because the functions are defined as async in the superclass unlike most commands (I can't really tell why), then any attempt to use these commands in a pipeline would result in a bug.

I will place these entreis in their own section

redis.asyncio.Sentinel.master_for
redis.asyncio.Sentinel.slave_for
redis.asyncio.sentinel.Sentinel.master_for
Expand Down
Loading