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

[py] moved static method get_path to SeleniumManager class #12554

Closed

Conversation

sandeepsuryaprasad
Copy link
Contributor

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Moved the static method get_path from DriverFinder class in common/driver_finder.py to SeleniumManager class in common/selenium_manager.py.

Motivation and Context

  • Its not common/standard practice (at least in Python) to have one single static method in a separate class.
  • Instead of having a single static method in a separate class definition, it is better to have it as a standalone function.
  • In this PR, I have moved static method get_path to SeleniumManager class itself. So that driver_finder.py and DriverFinder class could be eliminated.
  • Now all the methods related to SM binary and browser driver path are present in one single class, SeleniumManager.

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)

Checklist

  • I have read the contributing document.
  • [] My change requires a change to the documentation.
  • [] I have updated the documentation accordingly.
  • [] I have added tests to cover my changes.
  • All new and existing tests passed.

@diemol
Copy link
Member

diemol commented Aug 15, 2023

I believe we want to leave it as it is because we want to have more things in DriverFinder. cc @titusfortner

One thing I would appreciate is that an issue is raised before a PR like this is created. Changes like this should be discussed and therefore avoid unnecessary work. We are also in Slack if you want to discuss things you'd like to work on.

@titusfortner
Copy link
Member

I'm seeing some value in the Ruby side to making SeleniumManager class a lighter wrapper of the binary, and putting more of the driver finding logic into the DriverFinder class, like this - #12429

Not sure if it is the right direction; we should discuss

@isaulv
Copy link
Contributor

isaulv commented Aug 15, 2023

While I would agree with the sentiment that having a class with one static method is better written as a function, I think it's totally fine if we want to namespace these functions into a class as opposed to a separate module. But either way is fine, and the class way gives use more flexibility in the future if we want to add state, or other functionality.

@titusfortner
Copy link
Member

I'm closing this in favor of #13387

@sandeepsuryaprasad sandeepsuryaprasad deleted the driver-finder branch November 24, 2024 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants