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] websocket-client v.1.8.0 was added to setup.py #14187

Merged

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 25, 2024

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

  • 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.

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 codiumai-pr-agent-pro bot added dependencies Pull requests that update a dependency file Bug fix labels Jun 25, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 1
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review None

    Copy link
    Contributor

    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]

    -"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
    Copy link
    Contributor Author

    @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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    iampopovich commented Jun 25, 2024

    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
    Copy link
    Contributor Author

    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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    @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.bazel uses '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
    Copy link
    Member

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

    @iampopovich
    Copy link
    Contributor Author

    @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
    Copy link
    Member

    @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
    Copy link
    Contributor Author

    @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 titusfortner merged commit 5494108 into SeleniumHQ:trunk Jun 28, 2024
    13 of 14 checks passed
    @titusfortner
    Copy link
    Member

    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!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix dependencies Pull requests that update a dependency file Review effort [1-5]: 1
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants