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

feat: support list attribute #934

Closed
wants to merge 3 commits into from

Conversation

kkb912002
Copy link
Contributor

No description provided.

Copy link

linux-foundation-easycla bot commented Nov 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@KazuCocoa
Copy link
Member

Could you add test code like

def test_get_attribute_with_dict(self):
?
Please sign the CLA as well.

@KazuCocoa KazuCocoa changed the title support list attribute also feat: support list attribute Nov 28, 2023
@@ -60,7 +60,7 @@ def get_attribute(self, name: str) -> Optional[Union[str, Dict]]:
if attribute_value is None:
return None

if isinstance(attribute_value, dict):
if isinstance(attribute_value, dict) or isinstance(attribute_value, list):
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the return type must be updated as well and then (probably) mypy output fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

also isinstance may accept a tuple as the second argument

@mykola-mokhnach
Copy link
Contributor

Could you please provide some particular examples of where list is returned as attribute value? In java client all attributes are converted to strings: https://github.com/SeleniumHQ/selenium/blob/36585d189b2e9f2ced136a7e6c456ffe53604141/java/src/org/openqa/selenium/remote/RemoteWebElement.java#L149

@kkb912002
Copy link
Contributor Author

@mykola-mokhnach
If the 'value' field is parseable as JSON, it might be automatically parsed by the HTTP library used by the client in each language.
If there is a standard, we should follow it. This change follows the context of returning the dictionary as is. If it needs to be returned as a string, using json.dumps would be necessary for dict or list in the current situation.

ex) original value(string): '["abc", "def"]'
Parsed as List => ['abc', 'def']

  • str() => "['abc', 'def']" (value changed can't be parsed as json)
  • json.dumps() => '["abc", "def"]'

What should we do?

@mykola-mokhnach
Copy link
Contributor

If there is a standard, we should follow it. This change follows the context of returning the dictionary as is. If it needs to be returned as a string, using json.dumps would be necessary for dict or list in the current situation.

W3C Spec needs the result to be a string. I don't know why somebody added dict there as a special return case. I assume it would be smart to perform json.dumps on list values for this particular case without changing the return type. I would also do the same for dictionaries, but this will be a breaking change, so maybe in the next major client update

@KazuCocoa
Copy link
Member

@kkb912002 Could you give us what driver and element caused the case? Perhaps we could check the behavior as well

@kkb912002
Copy link
Contributor Author

@kkb912002 Could you give us what driver and element caused the case? Perhaps we could check the behavior as well

This is specific to occurrences within my custom UI library. I plan to make changes to comply with the W3C spec, returning a string. Feel free to close this request.

@KazuCocoa KazuCocoa closed this Nov 30, 2023
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.

3 participants