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] Address some build warnings #15157

Merged
merged 3 commits into from
Jan 25, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 25, 2025

PR Type

Bug fix, Enhancement


Description

  • Removed redundant exception message in Cookie.FromDictionary.

  • Added null-forgiving operator to avoid nullable warnings in CookieJar.

  • Updated obsolete CodeBase handling in FileUtilities with conditional checks.


Changes walkthrough 📝

Relevant files
Bug fix
Cookie.cs
Simplified exception handling in `Cookie.FromDictionary` 

dotnet/src/webdriver/Cookie.cs

  • Removed redundant exception message in ArgumentNullException.
+1/-1     
Enhancement
CookieJar.cs
Improved nullability handling in `CookieJar`                         

dotnet/src/webdriver/CookieJar.cs

  • Added null-forgiving operator to avoid nullable warnings.
+1/-1     
FileUtilities.cs
Updated obsolete `CodeBase` handling in `FileUtilities`   

dotnet/src/webdriver/Internal/FileUtilities.cs

  • Added conditional checks for obsolete CodeBase property.
  • Used pragma directives to suppress warnings for obsolete members.
  • +9/-2     

    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

    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 code assumes codeBase is not null when creating the directory path, but there's no fallback if it is null. Consider adding a fallback path.

    if (codeBase is not null)
    {
        currentDirectory = Path.GetDirectoryName(codeBase)!;
    }
    Null Handling

    Using the null-forgiving operator (!) may hide potential null reference issues. Consider adding explicit null checking instead.

    return Cookie.FromDictionary((Dictionary<string, object>)rawCookie!);

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 25, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect path parsing

    Create a new Uri from the codeBase string and use its LocalPath property to get the
    correct file path, as the raw CodeBase value might contain a URI format.

    dotnet/src/webdriver/Internal/FileUtilities.cs [199-202]

     if (codeBase is not null)
     {
    -    currentDirectory = Path.GetDirectoryName(codeBase)!;
    +    Uri uri = new Uri(codeBase);
    +    currentDirectory = Path.GetDirectoryName(uri.LocalPath)!;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical issue where direct use of CodeBase without URI parsing could cause path-related bugs, as CodeBase returns a URL format that needs proper conversion via Uri.LocalPath.

    9
    General
    Add descriptive exception message

    Add a descriptive exception message to help developers understand why the argument
    is invalid, as removing it makes debugging harder.

    dotnet/src/webdriver/Cookie.cs [280]

    -throw new ArgumentNullException(nameof(rawCookie));
    +throw new ArgumentNullException(nameof(rawCookie), "Cookie dictionary cannot be null");
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While adding a descriptive message could help with debugging, the PR deliberately removes the message to be consistent with standard .NET practices where ArgumentNullException often uses just the parameter name.

    3

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Nice to see less warnings ✅

    @nvborisenko nvborisenko merged commit 77796f4 into SeleniumHQ:trunk Jan 25, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-warnings branch January 26, 2025 11:42
    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