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

BizHawkClient: Fix typing mistake #3938

Merged
merged 3 commits into from
Sep 28, 2024
Merged

Conversation

Zunawe
Copy link
Collaborator

@Zunawe Zunawe commented Sep 14, 2024

What is this fixing or adding?

The type Iterable[int] was used in a few places to indicate some sort of list of integers. Specifically something that can be passed into the bytes constructor. It's almost accurate, but not specific enough. A generator that never stops iterating fits Iterable[int], but would be wrong here.

The actual type for what you can pass into bytes according to my IDE is Iterable[SupportsIndex] | SupportsIndex | SupportsBytes | ReadableBuffer. From what I can tell, Sequence[int] is a subset of Iterable[SupportsIndex], and it's still concise and descriptive.

How was this tested?

Did a few isinstances in a REPL and briefly opened an Emerald patch to make sure things didn't crash.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 14, 2024
@Zunawe Zunawe added the is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. label Sep 14, 2024
@Exempt-Medic Exempt-Medic added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. and removed is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. labels Sep 14, 2024
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

LGTM and hits all the instances of this

Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

An infinite generator would be a problem with almost everything typed with Iterable.
I think that's not really a problem with the Iterable typing. It's a problem with making and using an infinite generator.

But this change is not bad if you want it.

The thing I would be more concerned about is typing.List for read_list and guard_list and write_list. That makes it more difficult to get valid typing at the call site.
Those could be changed to Iterable or Sequence to make things easier for callers.

Generally, if you're not mutating the parameter in the function, you should use an immutable interface for the parameter type annotation.

@beauxq
Copy link
Collaborator

beauxq commented Sep 17, 2024

BTW, typing.Collection is another interface that would work for these that doesn't allow an infinite generator.
It would be less restrictive than Sequence.

But after thinking about it more, I think Sequence is the better option, but for a different reason:
A set fits both Iterable and Collection, but a set is an arbitrary order.
I think the order is important for all of these things. (You don't want bytes in any arbitrary order, right?)
So Sequence makes it more clear that the order is important.

So I would use Sequence for the outer type and the inner type (and even the return type, but that's less clear-cut).

async def guarded_read(ctx: BizHawkContext,
                       read_list: typing.Sequence[typing.Tuple[int, int, str]],
                       guard_list: typing.Sequence[typing.Tuple[int, typing.Sequence[int], str]]) -> typing.Optional[typing.Sequence[bytes]]:

and a similar change in the other functions

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 24, 2024
@Zunawe
Copy link
Collaborator Author

Zunawe commented Sep 27, 2024

Someone's mypy complained about Iterable for these reasons. I doubt people were considering passing an infinite generator into these functions, but I'd still call it a problem with the typing rather than strictly user error.

Sequence for the guard/read/write lists makes sense. I've changed that.

I'm not sure it makes sense to change the return type though, since we're just explicitly creating and returning a list. No reason to add extra abstraction.

@Berserker66 Berserker66 merged commit 8193fa1 into ArchipelagoMW:main Sep 28, 2024
18 checks passed
@Zunawe Zunawe deleted the bhc-typing branch September 28, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants