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] Enhance PrintOptions class to support for predefined and cus… #15144

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

Conversation

yvsvarma
Copy link
Contributor

@yvsvarma yvsvarma commented Jan 24, 2025

User description

PR Description

This PR implements the .NET version based on the original Java implementation: Original PR - Java.

The issue addressed in this PR revolves around the lack of predefined paper size options in the PrintOptions class for Selenium WebDriver in .NET. While the documentation suggests the ability to set commonly used paper sizes such as "A4," "Legal," and "Tabloid," the actual implementation lacked support for these features. This discrepancy created a usability gap and inconsistency with the expected functionality. The PR introduces predefined paper sizes, enhances usability through convenient methods, and aligns the .NET implementation with similar functionality in the Java WebDriver API.


PR Type

Enhancement, Tests


Description

  • Added predefined paper sizes (A4, Legal, Letter, Tabloid) to PrintOptions.

  • Introduced a default page size (A4) in the PrintOptions constructor.

  • Added a method to set custom or predefined page sizes with validation.

  • Implemented unit tests for default, predefined, and custom page sizes.


Changes walkthrough 📝

Relevant files
Enhancement
PrintOptions.cs
Add predefined paper sizes and default A4 to PrintOptions

dotnet/src/webdriver/PrintOptions.cs

  • Added predefined paper sizes (A4, Legal, Letter, Tabloid).
  • Set default page size to A4 in the constructor.
  • Introduced SetPageSize method with null validation.
  • Enhanced documentation for new properties and methods.
  • +28/-1   
    Tests
    PrintTest.cs
    Add unit tests for PrintOptions page sizes                             

    dotnet/test/common/PrintTest.cs

  • Added tests for default page size (A4).
  • Added tests for setting predefined page sizes.
  • Added tests for setting custom page sizes.
  • Added test for null page size validation.
  • +51/-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.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Code Duplication

    The null check in SetPageSize method duplicates the validation already present in the PageDimensions property setter. Consider removing the redundant check or consolidating the validation logic.

    if (pageSize == null)
    {
        throw new ArgumentNullException(nameof(pageSize), "Page size cannot be null.");
    }
    Documentation Issue

    The XML documentation comment for Margins property contains a typo ('doucment' instead of 'document'). While minor, documentation accuracy is important for API clarity.

    /// Gets or sets the margins for each page in the doucment.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix constructor initialization pattern

    The constructor is not properly initializing the page size. The property pageSize
    should be set directly instead of using PageDimensions to avoid potential null
    checks and exceptions.

    dotnet/src/webdriver/PrintOptions.cs [66-69]

     public PrintOptions()
     {
    -    this.PageDimensions = A4; // Default to A4 page size
    +    this.pageSize = A4; // Default to A4 page size
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a potential issue where using PageDimensions setter could throw an exception during object construction. Direct field initialization is safer and more appropriate for constructors.

    8
    General
    Remove redundant null validation

    The SetPageSize method duplicates the null check that's already present in the
    PageDimensions property. Remove the redundant validation to maintain DRY principles.

    dotnet/src/webdriver/PrintOptions.cs [120-127]

     public void SetPageSize(PageSize pageSize)
     {
    -    if (pageSize == null)
    -    {
    -        throw new ArgumentNullException(nameof(pageSize), "Page size cannot be null.");
    -    }
         this.PageDimensions = pageSize;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion correctly points out code duplication in null validation, as PageDimensions property already handles null checks. While valid, the impact is mainly on code maintainability rather than functionality.

    5

    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