Skip to content

Conversation

@nathanfallet
Copy link
Member

Description

Fixes #143

It makes me really worry about how we can trust mypy, since calling a non existent method does not raise any warning/error 😰

Pre-merge Checklist

  • I have described my change in the section above.
  • I have ran the ./scripts/format.sh and ./scripts/lint.sh scripts. My code is properly formatted and has no linting errors.
  • I have added my change to CHANGELOG.md under the [Unreleased] section.

@stephanlensky
Copy link
Member

We might have it configured wrong. I'll look into it later.

@stephanlensky stephanlensky merged commit 5ed36c1 into cdpdriver:main Jul 6, 2025
@nathanfallet nathanfallet deleted the fix/flash-point branch July 6, 2025 14:29
@stephanlensky
Copy link
Member

@nathanfallet it's because Tab and Connection (which Tab inherits from) both define __getattr__. Mypy isn't able to properly type check member access for classes which do that.

We should probably refactor at some point to no remove the usages of __getattr__. Not sure how big of a project that will be.

@nathanfallet
Copy link
Member Author

I understand the issue. Not sure it's a good thing to have this, since it breaks the safety. In kdriver, we're using a get() method when we need to dynamically access to properties (for example for Element to access to attributes), so we keep safety.

@stephanlensky
Copy link
Member

100% agree, it's from the original nodriver implementation. Specifically for Tab/Connection, I think it was just a hack to allow quick access to attributes on the TargetInfo without creating dedicated pass-through methods/properties.

I think we can probably just create dedicated @property methods to retrieve each of the attributes here https://github.com/stephanlensky/zendriver/blob/f4facfc1769824399533b7f64c8e0c6047d0c5a6/zendriver/cdp/target.py#L48-L74

and then remove the __getattr__ functions.

Solving this for Element will be a bit more challenging without breaking backwards compatibility.

@nathanfallet
Copy link
Member Author

Keep it somewhere in memory in case we decide to change the major version (from 0.x.x to 1.x.x); it could be a later improvement.

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