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 optional PromptUnload parameter when closing BrowsingContext and compiler helps #15254

Merged

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 8, 2025

User description

I started to add new optional bool PromptUnload parameter. And forgot to pass this option from caller!

Motivation and Context

I observed that we can define all parameters for command in ctor, doesn't matter they are required or optional. Now, when we want to add new optional parameter for command, compiler helps us.

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, Bug fix


Description

  • Refactored command parameter constructors to improve consistency.

  • Added PromptUnload parameter to CloseCommand for better unload handling.

  • Simplified optional parameter handling across BiDi modules.

  • Updated related tests and documentation for new parameter patterns.


Changes walkthrough 📝

Relevant files
Enhancement
29 files
BrowsingContextModule.cs
Refactored command parameter handling in BrowsingContextModule
+10/-71 
CaptureScreenshotCommand.cs
Simplified CaptureScreenshotCommandParameters constructor
+1/-8     
CloseCommand.cs
Added PromptUnload parameter to CloseCommand                         
+5/-2     
CreateCommand.cs
Refactored CreateCommandParameters to include optional fields
+1/-8     
GetTreeCommand.cs
Simplified GetTreeCommandParameters constructor                   
+1/-6     
HandleUserPromptCommand.cs
Refactored HandleUserPromptCommandParameters to include optional
fields
+1/-6     
LocateNodesCommand.cs
Simplified LocateNodesCommandParameters constructor           
+1/-8     
NavigateCommand.cs
Refactored NavigateCommandParameters to include optional fields
+1/-4     
PrintCommand.cs
Simplified PrintCommandParameters constructor                       
+1/-16   
ReloadCommand.cs
Refactored ReloadCommandParameters to include optional fields
+1/-6     
SetViewportCommand.cs
Simplified SetViewportCommandParameters constructor           
+1/-6     
AddInterceptCommand.cs
Refactored AddInterceptCommandParameters to include optional fields
+1/-6     
ContinueRequestCommand.cs
Simplified ContinueRequestCommandParameters constructor   
+1/-12   
ContinueResponseCommand.cs
Refactored ContinueResponseCommandParameters to include optional
fields
+1/-12   
NetworkModule.cs
Updated NetworkModule to use simplified parameter constructors
+5/-43   
ProvideResponseCommand.cs
Simplified ProvideResponseCommandParameters constructor   
+1/-12   
SetCacheBehaviorCommand.cs
Refactored SetCacheBehaviorCommandParameters to include optional
fields
+1/-4     
AddPreloadScriptCommand.cs
Simplified AddPreloadScriptCommandParameters constructor 
+1/-8     
CallFunctionCommand.cs
Refactored CallFunctionCommandParameters to include optional fields
+3/-14   
EvaluateCommand.cs
Simplified EvaluateCommandParameters constructor                 
+1/-8     
GetRealmsCommand.cs
Refactored GetRealmsCommandParameters to include optional fields
+1/-6     
ScriptModule.cs
Updated ScriptModule to use simplified parameter constructors
+4/-34   
SessionModule.cs
Refactored SessionModule to use simplified parameter constructors
+3/-10   
SubscribeCommand.cs
Simplified SubscribeCommandParameters constructor               
+1/-4     
UnsubscribeCommand.cs
Refactored UnsubscribeByAttributesCommandParameters to include
optional fields
+4/-5     
DeleteCookiesCommand.cs
Simplified DeleteCookiesCommandParameters constructor       
+1/-6     
GetCookiesCommand.cs
Refactored GetCookiesCommandParameters to include optional fields
+1/-6     
SetCookieCommand.cs
Simplified SetCookieCommandParameters constructor               
+1/-4     
StorageModule.cs
Updated StorageModule to use simplified parameter constructors
+3/-20   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Feb 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

    Breaking Change

    Changed Arguments property type in CallFunctionOptions from IEnumerable<object?> to IEnumerable<LocalValue?> which could break existing code that passes raw objects as arguments

    public IEnumerable<LocalValue?>? Arguments { get; set; }

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Add null validation checks for required method parameters to prevent NullReferenceExceptions

    Add null validation checks for required parameters in methods like
    CreateAsync(), NavigateAsync(), etc. to prevent potential
    NullReferenceExceptions. Use ArgumentNullException.ThrowIfNull() for validation.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContextModule.cs [31-33]

     public async Task<BrowsingContext> CreateAsync(ContextType type, CreateOptions? options = null)
     {
    +    ArgumentNullException.ThrowIfNull(type, nameof(type));
         var @params = new CreateCommandParameters(type, options?.ReferenceContext, options?.Background, options?.UserContext);
    • Apply this suggestion
    Low

    @nvborisenko nvborisenko merged commit 8f424e2 into SeleniumHQ:trunk Feb 8, 2025
    9 of 10 checks passed
    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.

    1 participant