You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR adds properties (getter/setter) for service_args to the service classes as requested in #12760.
🔧 Implementation Notes
Service args can be any sequence, not just a list.
🔄 Types of changes
Bug fix (backwards compatible)
New feature (non-breaking change which adds functionality and tests!)
PR Type
Enhancement
Description
• Add getter/setter properties for service_args across all WebDriver service classes
• Change parameter type from list[str] to Sequence[str] for better flexibility
• Implement private _service_args attribute with validation in setters
• Update documentation strings to reflect sequence type change
@service_args.setter
def service_args(self, value: Sequence[str]):
if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
+ if not all(isinstance(arg, str) for arg in value):+ raise TypeError("All service_args must be strings")
self._service_args = list(value)
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a missing validation step. While the type hint is Sequence[str], there is no runtime check to ensure all elements in the sequence are strings. Adding this check prevents potential TypeError exceptions when constructing command-line arguments for the subprocess, thus improving the code's robustness.
Medium
Possible issue
✅ Fix string validation logicSuggestion Impact:The commit directly implements the suggested fix by changing the validation condition from "not isinstance(value, Sequence) or isinstance(value, str)" to "isinstance(value, str) or not isinstance(value, Sequence)" on line 6.
code diff:
- if not isinstance(value, Sequence) or isinstance(value, str):+ if isinstance(value, str) or not isinstance(value, Sequence):
The validation logic incorrectly rejects strings because strings are sequences in Python. The condition should use isinstance(value, str) first to exclude strings, then check if it's a sequence. This prevents valid non-string sequences from being rejected.
@service_args.setter
def service_args(self, value: Sequence[str]):
- if not isinstance(value, Sequence) or isinstance(value, str):+ if isinstance(value, str) or not isinstance(value, Sequence):
raise TypeError("service_args must be a sequence")
self._service_args = list(value)
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the original validation logic not isinstance(value, Sequence) or isinstance(value, str) is flawed because a string is a sequence, causing the first part of the or to be false and the second to be true, thus always raising an error for strings. The proposed change isinstance(value, str) or not isinstance(value, Sequence) correctly handles this by checking for strings first.
✅ Convert sequence to list consistentlySuggestion Impact:The suggestion was directly implemented - the setter now converts the sequence to a list using list(value) instead of storing the sequence directly
The setter should convert the sequence to a list to maintain consistency with the internal storage format. The current implementation stores the sequence directly, which could lead to issues if a non-list sequence is provided.
@service_args.setter
def service_args(self, value: Sequence[str]):
if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out an inconsistency. The __init__ method initializes self._service_args as a list and uses .append(), but the setter allows any Sequence to be assigned. This could lead to runtime errors if a non-list sequence (like a tuple) is assigned and subsequent code expects a list. Converting to a list in the setter ensures type consistency for the internal attribute.
✅ Add string exclusion validation consistencySuggestion Impact:The suggestion's first part was implemented - the validation logic was updated to exclude strings by adding "or isinstance(value, str)" to the condition
code diff:
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
The setter validation is inconsistent with other service classes. It should also exclude strings from being accepted as valid sequences, since strings are sequences but not the intended type for service arguments.
@service_args.setter
def service_args(self, value: Sequence[str]):
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: This suggestion correctly points out two issues: the inconsistent validation that allows strings, and the need to convert the input to a list. Fixing this prevents potential errors and aligns the chrome service's behavior with all other service classes modified in this PR, improving overall consistency and robustness.
High
✅ Convert sequence to list consistentlySuggestion Impact:The suggestion was directly implemented - the setter now converts the sequence to a list using list(value) instead of storing the sequence directly
The setter should convert the sequence to a list to maintain consistency with the internal storage format. The current implementation directly assigns the sequence, which could lead to type inconsistencies since _service_args is expected to be a list.
@service_args.setter
def service_args(self, value: Sequence[str]):
if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that _service_args should consistently be a list, as it is initialized as one and other parts of the codebase may rely on its mutability (e.g., using .append()). Assigning a generic Sequence like a tuple could lead to runtime errors.
✅ Convert sequence to list assignmentSuggestion Impact:The suggestion was directly implemented - the setter now converts the input sequence to a list before assignment
Convert the sequence to a list before assignment to maintain consistency with the internal storage format and avoid potential issues with immutable sequences
@service_args.setter
def service_args(self, value: Sequence[str]):
if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly points out a potential issue. The __init__ method initializes _service_args as a list, and other methods in related classes mutate it using methods like .append(). The setter should also convert the incoming sequence to a list to maintain internal type consistency and prevent potential AttributeError if an immutable sequence (like a tuple) is passed.
✅ Add string exclusion validation consistencySuggestion Impact:The suggestion's first part was implemented - the validation logic was updated to exclude strings by adding "or isinstance(value, str)" to the condition
code diff:
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
The setter validation is inconsistent with other service classes. It should also exclude strings from being accepted as valid sequences, since strings are sequences but not the intended type.
@service_args.setter
def service_args(self, value: Sequence[str]):
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies two issues: the validation for service_args is incomplete as it allows strings, and the assigned value is not converted to a list. The proposed change aligns the implementation with other service classes in the PR, improving both robustness and consistency.
Medium
✅ Improve validation and conversion consistencySuggestion Impact:The commit partially implemented the suggestion by adding string exclusion to the validation logic, but did not include the list conversion part
code diff:
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
The setter validation should exclude strings and convert the sequence to a list for consistency with other service classes and internal storage format.
@service_args.setter
def service_args(self, value: Sequence[str]):
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
- self._service_args = value+ self._service_args = list(value)
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that the service_args setter is missing validation to exclude strings and does not convert the input sequence to a list. Applying this change makes the setter more robust and consistent with other service classes modified in this PR.
Convert the service_args to a list to ensure mutability since the property setter expects to assign a new value to this attribute, and sequences like tuples are immutable.
-self._service_args = service_args or []+self._service_args = list(service_args or [])
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly points out an inconsistency. While the code in chrome/service.py does not mutate self._service_args, other service classes in the PR convert service_args to a list to ensure mutability. Applying this change improves consistency across the codebase.
Low
✅ Convert sequence to listSuggestion Impact:The suggestion was directly implemented - the code was changed from `self._service_args = value` to `self._service_args = list(value)` exactly as suggested
Convert the value to a list before assignment to ensure the internal storage is mutable and consistent with the initialization pattern used elsewhere in the codebase.
if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
-self._service_args = value+self._service_args = list(value)
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that self._service_args should be a mutable list, as it's modified in the __init__ method. Converting the assigned value to a list in the setter ensures this, improving robustness and consistency with the initialization logic.
✅ Ensure service args mutabilitySuggestion Impact:The exact suggested change was implemented - wrapping service_args with list() to ensure mutability
code diff:
- self._service_args = service_args or []+ self._service_args = list(service_args or [])
Convert the input sequence to a list to ensure mutability for operations like append() used elsewhere in the code. This prevents potential issues when immutable sequences are passed.
-self._service_args = service_args or []+self._service_args = list(service_args or [])
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: This is a valuable suggestion. The code later appends to self._service_args, which requires a mutable sequence. If an immutable sequence like a tuple is passed for service_args, it would cause a runtime AttributeError. Converting to a list ensures mutability and prevents this bug.
Medium
✅ Prevent strings as sequence argumentsSuggestion Impact:The suggestion was directly implemented - the exact code change was made to add isinstance(value, str) check to prevent strings from being treated as valid sequence arguments
code diff:
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
The type check should exclude strings since they are sequences but shouldn't be treated as argument lists. Add a check to prevent strings from being accepted as valid sequences.
@service_args.setter
def service_args(self, value: Sequence[str]):
- if not isinstance(value, Sequence):+ if not isinstance(value, Sequence) or isinstance(value, str):
raise TypeError("service_args must be a sequence")
self._service_args = value
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that a string, being a sequence, would pass the isinstance(value, Sequence) check but is likely not the intended input for a list of arguments. Adding a check for isinstance(value, str) makes the validation more robust and prevents potential runtime errors.
Medium
Possible issue
✅ Fix inconsistent attribute assignmentSuggestion Impact:The suggestion was directly implemented - the line "self.service_args = service_args or []" was changed to "self._service_args = service_args or []" exactly as suggested
code diff:
- self.service_args = service_args or []+ self._service_args = service_args or []
The service_args assignment should use the private attribute _service_args to be consistent with the property implementation. This ensures the getter/setter pattern works correctly.
Why: The suggestion correctly identifies an inconsistency in py/selenium/webdriver/wpewebkit/service.py where self.service_args is used in __init__ instead of self._service_args. While functionally correct due to the setter, changing it improves consistency with other modified files in the PR.
I don't really see much benefit in allowing for sequences instead of forcing List as we have, but after looking into it every type in the Sequence library can be cast to List safely so LGTM.
Edit: Was #12760 reopened in the Slack, I see it's marked as not planned on the ticket?
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
🔗 Related Issues
Fixes #12760
Supersedes #14263
Supersedes #12767
💥 What does this PR do?
This PR adds properties (getter/setter) for
service_argsto the service classes as requested in #12760.🔧 Implementation Notes
Service args can be any sequence, not just a list.
🔄 Types of changes
PR Type
Enhancement
Description
• Add getter/setter properties for
service_argsacross all WebDriver service classes• Change parameter type from
list[str]toSequence[str]for better flexibility• Implement private
_service_argsattribute with validation in setters• Update documentation strings to reflect sequence type change
Changes walkthrough 📝
8 files
Update service_args type to SequenceAdd service_args property with getter/setterAdd service_args property with validationImplement service_args property and private attributeAdd service_args property with type validationImplement service_args getter/setter propertiesAdd service_args property with sequence validationImplement service_args property and private storage1 files
Add blank line formatting