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] [bidi] Add SetFiles command in Input module #15392

Merged
merged 4 commits into from
Mar 8, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Mar 8, 2025

User description

https://w3c.github.io/webdriver-bidi/#command-input-setFiles

Motivation and Context

Contributes to BiDi implementation.

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 SetFiles command to the BiDi Input module.

  • Implemented SetFilesCommand and its parameters for file upload.

  • Extended BrowsingContextInputModule and InputModule with SetFiles functionality.

  • Introduced SetFilesOptions for additional command configuration.


Changes walkthrough 📝

Relevant files
Enhancement
BrowsingContextInputModule.cs
Add `SetFiles` method to `BrowsingContextInputModule`       

dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContextInputModule.cs

  • Added SetFiles method to BrowsingContextInputModule.
  • Integrated SetFiles with the inputModule.
  • +5/-0     
    InputModule.cs
    Add `SetFilesAsync` method to `InputModule`                           

    dotnet/src/webdriver/BiDi/Modules/Input/InputModule.cs

  • Added SetFilesAsync method to InputModule.
  • Implemented command execution for SetFiles.
  • +7/-0     
    SetFilesCommand.cs
    Implement `SetFilesCommand` and parameters                             

    dotnet/src/webdriver/BiDi/Modules/Input/SetFilesCommand.cs

  • Created SetFilesCommand class for input.setFiles.
  • Added SetFilesCommandParameters for command parameters.
  • Introduced SetFilesOptions for optional configurations.
  • +30/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 8, 2025

    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

    Method Naming

    The method name 'SetFiles' is missing the 'Async' suffix, while it returns a Task. This is inconsistent with other async methods in the class like 'ReleaseActionsAsync'.

    public Task SetFiles(Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)
    {
        return inputModule.SetFilesAsync(context, element, files, options);
    }
    File Validation

    There's no validation for the file paths provided in the 'Files' parameter. The implementation should check if the files exist and are accessible before sending the command.

    internal record SetFilesCommandParameters(BrowsingContext.BrowsingContext Context, Script.ISharedReference Element, IEnumerable<string> Files) : CommandParameters;

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add Async suffix
    Suggestion Impact:The commit directly implemented the suggestion by renaming the SetFiles method to SetFilesAsync, following the C# naming convention for async methods

    code diff:

    -    public Task SetFiles(Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)
    +    public Task SetFilesAsync(Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)

    The method should be named with the "Async" suffix to follow the async method
    naming convention, as it returns a Task and calls an async method.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContextInputModule.cs [38-41]

    -public Task SetFiles(Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)
    +public Task SetFilesAsync(Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)
     {
         return inputModule.SetFilesAsync(context, element, files, options);
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies an inconsistency in method naming conventions. Methods that return Task should have the "Async" suffix according to C# conventions, especially since the method it calls (SetFilesAsync) follows this pattern. This improves code consistency and readability.

    Medium
    Learned
    best practice
    Add null parameter validation to prevent potential NullReferenceExceptions when handling required method parameters

    The SetFilesAsync method should validate that the element and files parameters
    are not null before using them. Add null checks using
    ArgumentNullException.ThrowIfNull() at the beginning of the method to ensure
    proper error handling and prevent potential NullReferenceExceptions.

    dotnet/src/webdriver/BiDi/Modules/Input/InputModule.cs [42-47]

     public async Task SetFilesAsync(BrowsingContext.BrowsingContext context, Script.ISharedReference element, IEnumerable<string> files, SetFilesOptions? options = null)
     {
    +    ArgumentNullException.ThrowIfNull(context, nameof(context));
    +    ArgumentNullException.ThrowIfNull(element, nameof(element));
    +    ArgumentNullException.ThrowIfNull(files, nameof(files));
    +    
         var @params = new SetFilesCommandParameters(context, element, files);
     
         await Broker.ExecuteCommandAsync(new SetFilesCommand(@params), options).ConfigureAwait(false);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • Update

    Sorry, something went wrong.

    @nvborisenko nvborisenko merged commit 86f5208 into SeleniumHQ:trunk Mar 8, 2025
    9 of 10 checks passed
    @nvborisenko nvborisenko deleted the bidi-setfiles branch March 8, 2025 17:34
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    None yet

    1 participant