SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.73k stars 8.02k forks source link

[py] websocket-client v.1.8.0 was added to setup.py #14187

Closed iampopovich closed 1 day ago

iampopovich commented 4 days ago

User description

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

according to request in #14184 websocket-client was added to setup.py version of client the same as in py/requirements.txt

Motivation and Context

Types of changes

Checklist


PR Type

Bug fix, Dependencies


Description


Changes walkthrough πŸ“

Relevant files
Dependencies
setup.py
Add websocket-client dependency to setup.py                           

py/setup.py
  • Added websocket-client version 1.8.0 to the install_requires list.
  • +1/-0     

    πŸ’‘ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-pro[bot] commented 4 days ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review [1-5] 1
    πŸ§ͺ Relevant tests No
    πŸ”’ Security concerns No
    ⚑ Key issues to review None
    codiumai-pr-agent-pro[bot] commented 4 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a version range for the new dependency to allow for minor updates and patches ___ **Consider using a version range for websocket-client instead of pinning it to a specific
    version. This can help avoid dependency conflicts and allow for minor updates and patches.** [py/setup.py [80]](https://github.com/SeleniumHQ/selenium/pull/14187/files#diff-722aeeec2821793eb6adf0c529cc7439c4b27ce78937cbb8840e94d4fc1c4017R80-R80) ```diff -"websocket-client==1.8.0", +"websocket-client>=1.8.0,<2.0.0", ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Using a version range instead of a specific version for dependencies can indeed help in managing minor updates and avoiding conflicts, which is a good practice in dependency management.
    7
    iampopovich commented 4 days ago

    @titusfortner I also noticed that the build script py/BUILD.bazel uses a loose requirement for the version of websocket-client. Should we enforce version 1.8.0 consistently across the entire project?

    Screenshot 2024-06-25 at 18 26 49
    titusfortner commented 4 days ago

    I agree they should all probably match.

    I don't know how python does things. In ruby if it is a transitive dependency, we leave the requirements loose to give the user more flexibility. I'm not sure if that matters here.

    iampopovich commented 4 days ago

    I don't know how python does things.

    It depends on the build system.

    With pip, if there's a conflict between libraries, the package manager will attempt to install the most suitable dependency that satisfies all requirements and avoids conflicts. Otherwise, developers will need to resolve dependency conflicts manually. In our case bazel uses pip to install requirements as it described here

    poetry, creates groups for dependencies, installing only versions that satisfy its requirements.

    The more loose requirements there are, the more flexibility the build system has to assemble a "working" configuration without conflicts. Managing a strictly defined set of dependencies that are tested thoroughly during build and testing phases makes configuration management easier. Updating dependencies becomes a more manageable process.

    Potentially, we might encounter version conflicts if other dependencies require websocket-client >= 1.8.0. I suggest trying to use version 1.8.0 and observing the build results. @titusfortner cc

    iampopovich commented 4 days ago

    I also noticed that the setup.py file does not include packages introduced after 2011, such as Safari (introduced in 2015). Should we add the missing packages to the packages section? And webdriver.support introduced twice

    Screenshot 2024-06-25 at 20 23 19
    titusfortner commented 4 days ago

    Python has had a lot of different fingers in the pot over the years, it is not surprising that there are things that have been missed as different people add things. Yes, please, update the things that make sense. Thanks.

    iampopovich commented 4 days ago

    @titusfortner I made changes to match dependencies in setup.py and BUILD.bazel. I think we should freeze dependencies to specific versions separately if needed later on. Right now, there are differences in library versions: for instance, py/BUILD.bazeluses 'trio~=0.17' in the 'requires' section, but py/requirements.txt uses 'trio>=0.20.2'. Maybe we should set up a single place for dependencies to get consistent information.

    titusfortner commented 3 days ago

    What do you suggest? I wish I knew more about bazel python rules in general.

    iampopovich commented 3 days ago

    @titusfortner I propose to accept the current pull request as is. Improvements for the build and dependencies will be considered later in a separate task. I also need to delve deeper into Bazel build rules

    titusfortner commented 2 days ago

    @iampopovich do these happen as part of your tooling or work flow?

    Merge branch 'trunk' into fix-14184-adding-websocket-client

    I can't see the results of the tests whenever an update is made 😁

    iampopovich commented 1 day ago

    @titusfortner I updated the branch to pull in the latest changes from trunk. If it's not necessary, I'll update the branch locally only.

    titusfortner commented 1 day ago

    GitHub will automatically squash & rebase for us, so if there aren't any changes that are likely to cause an issue, it isn't needed to keep merging trunk. It's only an issue because I want to merge it and it shows tests haven't run. 😁

    Thanks for the code!