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

17 and 18: Fix Target weirdness. #19

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Conversation

perrygoy
Copy link
Member

This PR:

  • Makes Targets log correctly after .first or any of the other property-based strategies in Playwright's toolkit.
  • Makes Targets look more like Locators by inheriting directly from them, while safeguarding access to their methods.

There's some whizbangery in this PR, but it works really nicely.

@perrygoy perrygoy requested a review from a team July 12, 2024 20:14
Copy link
Member Author

@perrygoy perrygoy left a comment

Choose a reason for hiding this comment

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

Some thoughts!

I noticed the mypy check failed, i'm not sure why it passed on pre-commit. I'll need to do some more dark magic to get those errors passing though.

@@ -87,11 +91,15 @@ def __repr__(self) -> str:
comma = ", "
return f"{self.name}{left_paren}{args}{comma}{kwargs}{right_paren}"

def __repr__(self) -> str:
"""Return the Target for representation."""
return repr(self.target)
Copy link
Member Author

Choose a reason for hiding this comment

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

This change handles the Target being correctly logged. Before, for a Target like:

t = Target.the("Ossuary door").located_by("graveyard > building door").first

Logging would be like:

Tor knocks on the first

rather than what it is now:

Tor knocks on the Ossuary door

Comment on lines 141 to 164
def __getattr__(self, name: str) -> _Manipulation:
def __getattribute__(self, name: str) -> _Manipulation:
"""Convert a Playwright Locator strategy into a Manipulation."""
if not hasattr(Locator, name):
msg = f"'{name}' is not a valid Playwright Locator strategy."
raise AttributeError(msg)
if hasattr(Locator, name):
attr = getattr(Locator, name)
is_property = type(attr) is property
r_type = getattr(attr, "__annotations__", {}).get("return")

if is_property or r_type is Locator or r_type == "Locator":
manipulation = _Manipulation(self, name)
self.manipulations.append(manipulation)
return manipulation

msg = f"'{name}' cannot be accessed until `found_by` is called."
raise TargetingError(msg)

manipulation = _Manipulation(self, name)
self.manipulations.append(manipulation)
return manipulation
return super().__getattribute__(name)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the aforementioned whizbangery.

We're using __getattribute__ for this work because we need to make sure we don't call the Locator methods until found_by actually returns it. Instead of calling the method, we append a _Manipulation to the manipulations list before returning it. That _Manipulation will wait to see if it's called and track the args/kwargs used, or it will pretend it's a Target when found_by is called.

@perrygoy
Copy link
Member Author

Note that typehints are failing because Microsoft's code hints that the return type is "Locator" instead of a typevar or Self.

Because of this, the methods inherited from Locator think they will be returning Locator instead of Target, which means mypy doesn't think we can access Target-specific attributes.

@perrygoy perrygoy merged commit 5028497 into trunk Jul 16, 2024
23 checks passed
@perrygoy perrygoy deleted the 17and18/fix-target-weirdness branch July 16, 2024 20:45
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.

1 participant