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

[dotnet] Public but non-user-related types can be invisible #14833

Closed

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 29, 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

Certain types are used only internally, but must be public because they are used across projects for testing.

Due to technical limitations, we cannot make these types internal.

This change will make those types invisible to users, which should prevent anyone taking a dependency on them.

Motivation and Context

Resolves #14813 the best we can.

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

enhancement


Description

  • Added EditorBrowsable attribute to internal classes to make them invisible to users.
  • Set EditorBrowsableState to Never for Base64UrlEncoder, PortUtilities, and ReturnedCookie classes.
  • Enabled nullable reference types in PortUtilities.

Changes walkthrough 📝

Relevant files
Enhancement
Base64UrlEncoder.cs
Hide Base64UrlEncoder class from user visibility                 

dotnet/src/webdriver/Internal/Base64UrlEncoder.cs

  • Added EditorBrowsable attribute to Base64UrlEncoder class.
  • Made the class invisible to users by setting EditorBrowsableState to
    Never.
  • +2/-0     
    PortUtilities.cs
    Hide PortUtilities class from user visibility                       

    dotnet/src/webdriver/Internal/PortUtilities.cs

  • Added EditorBrowsable attribute to PortUtilities class.
  • Made the class invisible to users by setting EditorBrowsableState to
    Never.
  • Enabled nullable reference types.
  • +5/-1     
    ReturnedCookie.cs
    Hide ReturnedCookie class from user visibility                     

    dotnet/src/webdriver/Internal/ReturnedCookie.cs

  • Added EditorBrowsable attribute to ReturnedCookie class.
  • Made the class invisible to users by setting EditorBrowsableState to
    Never.
  • +2/-0     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Reference
    The forced null assertion operator (!) on portSocket.LocalEndPoint assumes the value won't be null, but this may not be guaranteed. Consider adding null check before the cast.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Ensure proper resource cleanup by using disposable objects within a 'using' block

    The socket should be properly disposed using a 'using' statement to ensure resources
    are released, even if an exception occurs.

    dotnet/src/webdriver/Internal/PortUtilities.cs [44-53]

    -Socket portSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    -try
    +using (Socket portSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
     {
         IPEndPoint socketEndPoint = new IPEndPoint(IPAddress.Any, 0);
         portSocket.Bind(socketEndPoint);
         socketEndPoint = (IPEndPoint)portSocket.LocalEndPoint!;
         listeningPort = socketEndPoint.Port;
     }
    -finally
    -{
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a 'using' statement for Socket disposal is a critical best practice that ensures proper resource cleanup and prevents memory leaks, especially when dealing with system resources like network sockets.

    8

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Member

    It should be internal.

    @RenderMichael
    Copy link
    Contributor Author

    In that case, what do you think of the suggestion I made in #14813? We can't make it internal in a normal way, so we have to get creative. We can also copy-paste the code into the tests.

    If we're not willing to make too much magic, then I think we should make this PR. The current state is the worst of all worlds: users may get intellisense suggestions for these types and start using them.

    @nvborisenko
    Copy link
    Member

    so we have to get creative

    No creativity, it should be internal. Build system is not yet ready? Yes! But it should be internal.

    @nvborisenko
    Copy link
    Member

    Maximum what I can do: https://seleniumhq.slack.com/archives/CBH302726/p1732918073634829.

    I am closing it because of it is a strange workaround, not a proper solution.

    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.

    [dotnet] [🚀 Feature]: Consider using the new Base64Url
    2 participants