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 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
Follow up to PR #14528, to avoid os.getenv raising TypeError in env_path when driver_path_env_key is not passed to Service constructor.
This happens, for example, with the WPEWebKit driver, which currently ignores driver_path_env_key, as the executable WPEWebDriver is usually available on $PATH (which is set either natively or through other test scripts).
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Code Style Consider making DRIVER_PATH_ENV_KEY a class variable with default value instead of instance variable, since it represents a constant configuration value
Using None instead of an empty string better represents the absence of an environment variable key
The empty string default for DRIVER_PATH_ENV_KEY could cause issues when checking environment variables. Consider using None as the default value to clearly indicate no environment key is specified.
-self.DRIVER_PATH_ENV_KEY = driver_path_env_key or ""+self.DRIVER_PATH_ENV_KEY = driver_path_env_key or None
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Using None as a default value is more semantically correct for representing the absence of an environment variable key, making the code's intent clearer and preventing potential issues when checking environment variables.
Explore these optional code suggestions:
Category Suggestion Score General
Using None instead of an empty string better represents the absence of an environment variable key
The empty string default for DRIVER_PATH_ENV_KEY could cause issues when checking environment variables. Consider using None as the default value to clearly indicate no environment key is specified.
-self.DRIVER_PATH_ENV_KEY = driver_path_env_key or ""+self.DRIVER_PATH_ENV_KEY = driver_path_env_key or None
* [ ] **Apply this suggestion**
Suggestion importance[1-10]: 7
Why: Using None as a default value is more semantically correct for representing the absence of an environment variable key, making the code's intent clearer and preventing potential issues when checking environment variables.
7
Follow up to PR SeleniumHQ#14528, to avoid `os.getenv` raising `TypeError`
in `env_path` when `driver_path_env_key` is not passed to `Service`
constructor.
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.
lauromoura
force-pushed
the
fix_driver_path_env_key_type
branch
from
User description
Description
Follow up to PR #14528, to avoid
os.getenvraisingTypeErrorinenv_pathwhendriver_path_env_keyis not passed toServiceconstructor.This happens, for example, with the WPEWebKit driver, which currently ignores
driver_path_env_key, as the executableWPEWebDriveris usually available on$PATH(which is set either natively or through other test scripts).Types of changes
Checklist
PR Type
Bug fix
Description
driver_path_env_keyin theServiceclass docstring.DRIVER_PATH_ENV_KEYto default to an empty string if not provided, preventingTypeErrorwhenos.getenvis called.driver_path_env_key.Changes walkthrough 📝
service.py
Ensure DRIVER_PATH_ENV_KEY defaults to an empty stringpy/selenium/webdriver/common/service.py
driver_path_env_key.DRIVER_PATH_ENV_KEYto an empty string.