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

[java]: encapsulate additionalCommands with getter method #14816

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Nov 27, 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

Solves #14811
Changed the scope of additionalCommands under HttpCommandExecutor to protected

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


Description


Changes walkthrough 📝

Relevant files
Bug fix
HttpCommandExecutor.java
Change scope of `additionalCommands` to `protected`           

java/src/org/openqa/selenium/remote/HttpCommandExecutor.java

  • Changed the access modifier of additionalCommands from private to
    protected.
  • This change allows subclasses to access additionalCommands.
  • +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Sorry, something went wrong.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    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

    Access Control
    Changing field visibility from private to protected exposes internal implementation details. Verify that this change doesn't break encapsulation principles or expose sensitive command information to untrusted subclasses.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Restrict access modifier of a field to maintain proper encapsulation while preserving inheritance functionality

    The client field is declared as public but should be protected since it's part of
    the internal implementation. Change its access modifier to protected to maintain
    encapsulation while still allowing subclass access.

    java/src/org/openqa/selenium/remote/HttpCommandExecutor.java [47]

    -public final HttpClient client;
    +protected final HttpClient client;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a potential encapsulation issue. Making the 'client' field protected instead of public aligns with good OOP practices by restricting access while still allowing inheritance functionality, similar to how other fields in the class are handled.

    7

    💡 Need additional feedback ? start a PR chat

    Sorry, something went wrong.

    @VietND96
    Copy link
    Member

    If it is possible, try to add unit test also.

    @VietND96 VietND96 linked an issue Nov 27, 2024 that may be closed by this pull request
    @navin772 navin772 changed the title [java]: change scope of additionalCommands to protected [java]: encapsulate additionalCommands with getter method Nov 27, 2024
    @diemol
    Copy link
    Member

    diemol commented Nov 27, 2024

    @mykola-mokhnach do you think you'd need a method to remove commands at any point in the future?

    @mykola-mokhnach
    Copy link

    @mykola-mokhnach do you think you'd need a method to remove commands at any point in the future?

    I did not think about such far future :)
    Maybe not...

    @diemol diemol merged commit 59aa1a0 into SeleniumHQ:trunk Nov 27, 2024
    1 check passed
    @navin772 navin772 deleted the appium-2 branch November 27, 2024 12:36
    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.

    [🚀 Feature]: Better compatibility with Appium #2
    5 participants