Skip to content

Update locating element log level to warning #1073

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

Merged
merged 1 commit into from
Jun 11, 2025

Conversation

visagang
Copy link
Contributor

@visagang visagang commented Jun 10, 2025

PR Type

Enhancement


Description

• Add ignore_if_not_found parameter to element locating functionality
• Update JSON schema to include new boolean parameter for error handling
• Enhance element location control with optional error suppression


Changes walkthrough 📝

Relevant files
Enhancement
ElementLocatingArgs.cs
Add ignore_if_not_found property to ElementLocatingArgs   

src/Infrastructure/BotSharp.Abstraction/Browsing/Models/ElementLocatingArgs.cs

• Add ignore_if_not_found property with JSON serialization attribute

Enable optional error suppression for element location failures

+1/-0     
Configuration changes
util-web-locate_element.json
Update function schema with ignore_if_not_found parameter

src/Plugins/BotSharp.Plugin.WebDriver/data/agents/6745151e-6d46-4a02-8de4-1c4f21c7da95/functions/util-web-locate_element.json

• Add ignore_if_not_found parameter to function schema
• Define
boolean type with description for error logging control

+4/-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

    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

    Missing Documentation

    The new IgnoreIfNotFound property lacks XML documentation comments explaining its purpose and behavior, while other properties in the class have proper documentation.

    [JsonPropertyName("ignore_if_not_found")]
    public bool IgnoreIfNotFound { get; set; }
    Schema Inconsistency

    The JSON schema parameter name ignore_if_not_found uses snake_case while the C# property uses PascalCase IgnoreIfNotFound. This naming inconsistency could cause mapping issues.

    "ignore_if_not_found": {
      "type": "boolean",
      "description": "ignore error logging if element not exists"
    }

    @GGHansome
    Copy link

    Auto Review Result:

    Code Review Summary

    Summary of Changes: The provided code introduces a new attribute [JsonPropertyName("ignore_if_not_found")] to the existing IgnoreIfNotFound property in the ElementLocatingArgs class. This change is intended to ensure that the IgnoreIfNotFound boolean property is serialized and deserialized with the JSON property name ignore_if_not_found, likely to comply with a specific API or external data format.

    Identified Issues

    Issue 1: [Code Clarity]

    • Description: The use of [JsonPropertyName] is good for serialization consistency but may introduce confusion if it does not match existing naming conventions or is not documented.

    • Suggestion: Ensure this attribute is documented, especially if it affects the external interface of the application. Additionally, verify that existing tests will cover this change to avoid serialization issues.

      // Example documentation comment above the property
      /// <summary>
      /// Gets or sets a value indicating whether the operation should ignore the element if not found, 
      /// serialized as "ignore_if_not_found".
      /// </summary>
      [JsonPropertyName("ignore_if_not_found")]
      public bool IgnoreIfNotFound { get; set; }

    Overall Assessment

    The code is a straightforward improvement to ensure that JSON serialization matches external expectations. The introduction of a [JsonPropertyName] attribute is appropriate and increases flexibility when interacting with JSON-based systems. However, I recommend adding documentation to this class property to clarify its purpose and ensure consistency across the codebase. Additionally, checking or updating unit tests related to JSON serialization could prevent unforeseen issues.

    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @Oceania2018 Oceania2018 merged commit 6f0754c into SciSharp:master Jun 11, 2025
    4 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.

    3 participants