[py] Encapsulated Geo Location co-ordinates using getters/setters

[sandeepsuryaprasad]

User description

🔗 Related Issues

💥 What does this PR do?

Encapsulated Geo Location co-ordinates using getters/setters

🔧 Implementation Notes

Rather than having to validate the instance attributes, in this case Geo Location Co-ordinates inside constructor, It's a good idea and a standard practice to encapsulate all geo-location co-ordinates inside getters/setters, validate through setter methods. It is very clear from each setter method as to what is the attribute that we are setting and what are upper and lower limits to that attribute.

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Other

---

Description

• Refactored GeolocationCoordinates class to use property decorators

• Moved validation logic from constructor to setter methods

• Improved encapsulation with private attributes and getters/setters

---

Diagram Walkthrough

flowchart LR
  A["Constructor validation"] -- "refactored to" --> B["Property setters"]
  B --> C["Private attributes"]
  C --> D["Getter methods"]

Loading

File Walkthrough

	Relevant files
Enhancement	emulation.py Property-based validation for GeolocationCoordinates          py/selenium/webdriver/common/bidi/emulation.py Removed validation logic from __init__ method Added property decorators for all coordinate attributes Implemented setter methods with validation for each property Changed attributes to private with underscore prefix +70/-15

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 5678 - Not compliant Non-compliant requirements: • Fix ChromeDriver connection failure error that occurs after first instance • Resolve "ConnectFailure (Connection refused)" error for subsequent ChromeDriver instances • Ensure proper ChromeDriver instantiation on Ubuntu 16.04.4 with Chrome 65.0.3325.181 1234 - Not compliant Non-compliant requirements: • Fix JavaScript execution in link's href attribute when using click() method • Ensure compatibility with Firefox 42.0 • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Error The altitude_accuracy setter validation checks if altitude is None, but this creates a dependency issue during initialization when altitude_accuracy is set before altitude in the constructor. if value is not None and self.altitude is None: raise ValueError("altitude_accuracy cannot be set without altitude") Missing Validation The altitude setter has no validation logic, allowing any value including invalid ones, while the original constructor had validation for altitude_accuracy dependency. @altitude.setter def altitude(self, value): self._altitude = value

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use private attribute for validation The validation logic references self.altitude which could trigger the altitude property getter, potentially causing issues if altitude is not yet initialized. Use the private attribute self._altitude instead for internal validation checks. py/selenium/webdriver/common/bidi/emulation.py [102-108] @altitude_accuracy.setter def altitude_accuracy(self, value): - if value is not None and self.altitude is None: + if value is not None and self._altitude is None: raise ValueError("altitude_accuracy cannot be set without altitude") if value is not None and value < 0.0: raise ValueError("Altitude accuracy must be >= 0.0") self._altitude_accuracy = value Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential issue where the altitude_accuracy setter depends on the altitude property, which could fail if initialization order changes; using the private _altitude attribute makes the code more robust.	Low
Update

[navin772]

I don't think this is required and feels a bit overkill for just validating the coordinates.

If you think of any specific case this would be required, let me know.

[sandeepsuryaprasad]

@navin772 The co-ordinates gets validated only while creating an instance. But one can change the attributes even after creating instance.. e.g.

coords = GeolocationCoordinates(45.5, -122.4194, accuracy=10.0)
coords.latitude = "45.5"  # bad value. this will not be caught in the current code on the trunk and this does't go through constructor.

So it's Safe and standard practice to have it inside a private attribute and provide access to the user via Getter and Setter interface. Definitely it's not a good practice to have all those attributes validated in constructor. By having separate set of getters/setters the code is more clean and readable.

[sandeepsuryaprasad]

setters are meant for validating the values before attributes are being set. In this case we are actually validating the values of co-ordinates before setting it. So obvious choice is to use setters, which I don't feel it as overkill. why would someone use setters if it is not for validation? Let me know your thoughts on this. Thanks!

[navin772]

But one can change the attributes even after creating instance

This seems like a valid argument, what do you think @cgoldberg?

[cgoldberg]

If we care about enforcing validation of coordinates after the class is instantiated, this PR is good. I don't have a strong preference, but would be fine merging this.

[cgoldberg]

We should remove the capitalization in all error messages when referring to an attribute name:

i.e. change:

"Longitude must be between -180.0 and 180.0"

to

"longitude must be between -180.0 and 180.0"

[sandeepsuryaprasad]

@cgoldberg done! Removed capitalisation of all attributes in error messages.

[cgoldberg]

LGTM