Skip to content

pyAutoGui: Correct Return Type of position() to Match Actual Behavior #11267

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

Merged

Conversation

codekoriko
Copy link
Contributor

@codekoriko codekoriko commented Jan 10, 2024

Point being a Tuple of float it does not match the actual behavior of the position() function:

def position(x=None, y=None):
    """
    Returns the current xy coordinates of the mouse cursor as a two-integer tuple.

    Args:
      x (int, None, optional) - If not None, this argument overrides the x in
        the return value.
      y (int, None, optional) - If not None, this argument overrides the y in
        the return value.

    Returns:
      (x, y) tuple of the current xy coordinates of the mouse cursor.

    NOTE: The position() function doesn't check for failsafe.
    """
    posx, posy = platformModule._position()
    posx = int(posx)
    posy = int(posy)
    if x is not None:  # If set, the x parameter overrides the return value.
        posx = int(x)
    if y is not None:  # If set, the y parameter overrides the return value.
        posy = int(y)
    return Point(posx, posy)

@AlexWaygood AlexWaygood changed the title Correct Return Type of position() to Match Actual Behavior pyAutoGui: Correct Return Type of position() to Match Actual Behavior Jan 10, 2024
@AlexWaygood AlexWaygood requested a review from Avasam January 10, 2024 16:49

This comment has been minimized.

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

My concern is that this will cause additional casting being needed when a Point is expected and you loose property access:

from math import hypot

def distance_from_origin(point: Point) -> float:
    return hypot(point.x, point.y)

int_point = position(5, 10)

distance = distance_from_origin(int_point)  # (variable) int_point: tuple[int, int]
# Argument of type "tuple[int, int]" cannot be assigned to parameter "point" of type "Point" in function "distance_from_origin"
#  "tuple[int, int]" is incompatible with "Point" Pylance(reportGeneralTypeIssues)

# Argument 1 to "distance_from_origin" has incompatible type "Tuple[int, int]"; expected "Point" Mypy(arg-type)

I think that by using a covariant Generic this can be solved:

from typing import Generic, NamedTuple, TypeVar

_T = TypeVar("_T", int, float, default=float, covariant=True)

class Point(NamedTuple, Generic[_T]):
    x: _T
    y: _T

def position(x: int | None = None, y: int | None = None) -> Point[int]: ...

###

# Downstream code example

from math import hypot

def distance_from_origin(point: Point):  # Same as Point[float]
    return hypot(point.x, point.y)

int_point = position(5, 10)

distance = distance_from_origin(int_point)

This makes possible to pass a Point[int] to a method expecting a Point[float]. And still disallows passing a Point[float] to a method expecting a Point[int].

Once implemented, we could probably improve other methods that are also known to always return a Point[int]. I vaguely remember screen positions being a float only on a single platform (linux?).

The use of default reduces required changes (especially downstream), given this is an edge case. I'm not sure what's the current state of https://peps.python.org/pep-0696/ support. But typeshed's tests should fail if a supported type-checker is not ready for TypeVar defaults.

(CC @AlexWaygood in case you see any issue with my proposal, but it seems sound to me and my quick testing)

@AlexWaygood
Copy link
Member

CC @AlexWaygood in case you see any issue with my proposal, but it seems sound to me and my quick testing

(I'm super busy this week unfortunately, so probably won't be able to take a proper look at this until after the 20th, but I trust your judgement!)

@JelleZijlstra
Copy link
Member

I don't think we're quite ready for PEP 696 in typeshed. Until then, the current annotation is probably best; int is a subtype of float in the type system, after all.

@Avasam Avasam added the status: deferred Issue or PR deferred until some precondition is fixed label Mar 7, 2024
@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Mar 20, 2024
@srittau
Copy link
Collaborator

srittau commented Mar 20, 2024

TypeVar defaults are now available in typeshed.

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Oct 25, 2024

@codekoriko Are you willing to update this PR to make use of generic defaults as per #11267 (review) ? If not, I can probably take it over.

@codekoriko
Copy link
Contributor Author

@codekoriko Are you willing to update this PR to make use of generic defaults as per #11267 (review) ? If not, I can probably take it over.

please go ahead 👍🏻

@JelleZijlstra
Copy link
Member

@Avasam are you still interested in continuing this PR?

@Avasam Avasam self-assigned this Dec 28, 2024
@Avasam
Copy link
Collaborator

Avasam commented Dec 28, 2024

I forgot about this. I just applied my previous suggestion.

Although, looking back into this. It looks like PyAutoGUI's Point is only ever used with int, and only ever exposed through position's return type.

So OP's original PR might've been fine as-is.

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 5a10be1 into python:main Dec 30, 2024
49 checks passed
hoel-bagard pushed a commit to hoel-bagard/typeshed that referenced this pull request Jan 5, 2025
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.

5 participants