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] Annotate nullable reference types on input devices #14804

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

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

Annotates nullable reference types on InputDevice and derived types. Implement modernization, simplification, and more XML documentation along the way.

Motivation and Context

Contributes to #14640

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, documentation


Description

  • Enabled nullable reference types across multiple input device classes to improve null safety.
  • Modernized code by using auto-properties and expression-bodied members for simplicity.
  • Added XML documentation for constructors to specify exceptions thrown.
  • Improved exception handling and documentation for better code clarity and maintenance.

Changes walkthrough 📝

Relevant files
Enhancement
InputDevice.cs
Annotate nullable types and modernize InputDevice class   

dotnet/src/webdriver/Interactions/InputDevice.cs

  • Enabled nullable reference types.
  • Replaced field with auto-property for DeviceName.
  • Simplified method implementations using expression-bodied members.
  • Added XML documentation for exceptions.
  • +8/-16   
    KeyInputDevice.cs
    Annotate nullable types and simplify KeyInputDevice           

    dotnet/src/webdriver/Interactions/KeyInputDevice.cs

  • Enabled nullable reference types.
  • Simplified property and method implementations.
  • Added XML documentation for exceptions.
  • +10/-14 
    PointerInputDevice.cs
    Enhance PointerInputDevice with nullable annotations         

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs

  • Enabled nullable reference types.
  • Converted fields to readonly where applicable.
  • Simplified property and method implementations.
  • Improved exception handling and documentation.
  • +60/-114
    WheelInputDevice.cs
    Enhance WheelInputDevice with nullable annotations             

    dotnet/src/webdriver/Interactions/WheelInputDevice.cs

  • Enabled nullable reference types.
  • Converted fields to readonly where applicable.
  • Simplified property and method implementations.
  • Improved exception handling and documentation.
  • +23/-50 

    💡 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 ToString() method assumes target is not null when origin is CoordinateOrigin.Element. This could cause NullReferenceException if target is null.

    Code Duplication
    The ConvertElement() method is duplicated between PointerInputDevice and WheelInputDevice classes. Consider extracting to a shared helper class.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add parameter validation to prevent null reference exceptions

    Add null check for properties parameter in the constructor to avoid potential
    NullReferenceException when accessing eventProperties later.

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs [427-432]

     public PointerDownInteraction(InputDevice sourceDevice, MouseButton button, PointerEventProperties properties)
         : base(sourceDevice)
     {
         this.button = button;
    -    this.eventProperties = properties;
    +    this.eventProperties = properties ?? throw new ArgumentNullException(nameof(properties));
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding null validation for the properties parameter is crucial as it prevents potential NullReferenceException in the ToDictionary method where eventProperties is used.

    8
    Add null check for required parameter to prevent runtime errors

    Add null check for target parameter in ConvertElement() since it's marked as
    nullable but the method assumes it's non-null.

    dotnet/src/webdriver/Interactions/PointerInputDevice.cs [584-588]

     private Dictionary<string, object> ConvertElement()
     {
    +    if (this.target == null)
    +    {
    +        throw new ArgumentNullException(nameof(target));
    +    }
         if (this.target is IWebDriverObjectReference element)
         {
             return element.ToDictionary();
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The ConvertElement method assumes target is non-null but it's marked as nullable. Adding an explicit null check improves error handling and prevents runtime exceptions.

    7

    💡 Need additional feedback ? start a PR chat

    {
    return this.CreatePause(TimeSpan.Zero);
    }
    public Interaction CreatePause() => this.CreatePause(TimeSpan.Zero);
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Not for method, topic for editorconfig.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Do you think we need a consistent rule here? I like to use method expressions if the method contains no real logic, just alternate parameters or for a wrapper.

    If we want to always use method bodies, I can revert this change.

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Yes please revert, for methods. This question is out of scope of this PR.

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I like to use method expressions if the method contains no real logic, just alternate parameters or for a wrapper.

    I like to preserve a placeholder via { } to add more code inside when a method becomes to not if the method contains no real logic.

    It is more for internal conventions (editorconfig topic), at this time let us be less destructive.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I completely get that, it allows for much smaller diffs. I'll try to stick to that convention. The editorconfig PR is updated with the latest thinking btw :)

    }

    if (elementReference == null)
    if (this.target is IWrapsElement { WrappedElement: IWebDriverObjectReference wrappedElement })
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    So modernized! I am old-school (I am not alone).

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I remember I used to jump on every modern innovation, and turn code into a mess just to use new features. These days I pick and choose.

    I really like the property pattern matching personally, this method is now much shorter.

    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.

    2 participants