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

soe: change FlagProtocol to match default typing #2

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

beauxq
Copy link
Owner

@beauxq beauxq commented Jan 14, 2024

What is this fixing or adding?

change FlagProtocol to match default typing for PR ArchipelagoMW#2173

(and default isn't needed in FlagsProtocol)

This wouldn't be needed if we could use ReadOnly in protocols (as mentioned in one line of PEP 705.
It should be

    value: ReadOnly[object]
    default: ReadOnly[object]

since the the types of those don't matter at all for FlagProtocol

But we don't have ReadOnly yet, so LSP says the typing has to match Option.

How was this tested?

mypy, pyright

@beauxq
Copy link
Owner Author

beauxq commented Jan 14, 2024

Actually, now I'm seeing that mypy doesn't like anything with this FlagProtocol usage after the ArchipelagoMW#2173 change...

I think it's because of the bug discussed in that PR.

This bug is frustrating...

@beauxq
Copy link
Owner Author

beauxq commented Jan 14, 2024

I'm questioning whether ArchipelagoMW#2173 is worth it...

@black-sliver
Copy link

That change to the Flags Protocol looks good regardless. Should I incorporate it into my open SoE PR and we close this?

Not sure about 2173. I think having correct typing in Options would be awesome, but it would be nicer if it would "just work". Python being too flexible for static analysis here, I think.

@beauxq
Copy link
Owner Author

beauxq commented Jan 15, 2024

Should I incorporate it into my open SoE PR and we close this?

Well, the default: Union[int, Literal["random"]] doesn't match core typing without 2173, so I think it might cause typing errors if you make that change in your other PR. (I haven't tested it.)

But the removing default from FlagsProtocol could be done in your other PR.

@beauxq
Copy link
Owner Author

beauxq commented Jan 15, 2024

Python being too flexible for static analysis here, I think.

Not even Any can solve this problem.

https://mypy-play.net/?mypy=latest&python=3.11&gist=ef5fea9cb8e11b10cb1568ce5dc12fdc&flags=strict

So, "flexible" or "dynamic" has nothing to do with it.

Copy link

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Well, the default: Union[int, Literal["random"]] doesn't match core typing without 2173, so I think it might cause typing errors if you make that change in your other PR. (I haven't tested it.)

You are right. Merging that change into 2173 and keeping ArchipelagoMW/ArchipelagoMW#2724 as-is is the better idea. (And I'd prefer changing both protocols in the same PR.)

So, "flexible" or "dynamic" has nothing to do with it.

What I mean is that python allows this kind of thing by not caring about the type at all, but for mypy it's complicated to reason the type of a ClassVar that gets overridden.

@beauxq beauxq merged commit ae7604d into option-default-typing Jan 15, 2024
18 checks passed
beauxq pushed a commit that referenced this pull request Mar 9, 2024
some cleaning, typing, consistency fixing
beauxq pushed a commit that referenced this pull request Aug 17, 2024
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.

2 participants