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

toml: warn about upcoming change enforcing string to have quotes #14491

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Sep 12, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This adds a warning stated in #14470 which tells about the future change. Strings in TOML now need to have quotes

Motivation and Context

Warning to help migration from old TOML parser to the new parser

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 a logger to the TomlConfig class to provide warnings.
  • Implemented a warning message to inform users about the upcoming requirement for strings in TOML files to be enclosed in quotes.
  • This change is intended to help users transition smoothly to the new TOML parser that will enforce this requirement.

Changes walkthrough 📝

Relevant files
Enhancement
TomlConfig.java
Add warning for future TOML string quoting requirement     

java/src/org/openqa/selenium/grid/config/TomlConfig.java

  • Added a logger to the TomlConfig class.
  • Introduced a warning message about future TOML parser changes.
  • Warns users to use quotes for strings in TOML files.
  • +4/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logging Improvement
    The warning message is logged every time a TOML file is parsed, which might lead to excessive logging. Consider adding a flag to log this warning only once per JVM session.

    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 12, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a conditional check for showing the warning message

    Consider adding a version check or feature flag to conditionally show the warning,
    allowing for easier transition when the new TOML parser is implemented.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [46]

    -LOG.warning("Please use quotes to denote strings. Upcoming TOML parser will require this and unquoted strings will throw an error in the future");
    +if (shouldWarnAboutTomlQuotes()) {
    +    LOG.warning("Please use quotes to denote strings. Upcoming TOML parser will require this and unquoted strings will throw an error in the future");
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a conditional check for the warning message is a proactive enhancement that allows for smoother transitions when the new TOML parser is implemented, making it a valuable improvement.

    8
    Maintainability
    Extract the warning message into a constant

    Consider using a constant for the warning message to improve maintainability and
    allow for easier updates in the future.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [46]

    -LOG.warning("Please use quotes to denote strings. Upcoming TOML parser will require this and unquoted strings will throw an error in the future");
    +private static final String TOML_QUOTE_WARNING = "Please use quotes to denote strings. Upcoming TOML parser will require this and unquoted strings will throw an error in the future";
    +...
    +LOG.warning(TOML_QUOTE_WARNING);
     
    Suggestion importance[1-10]: 7

    Why: Extracting the warning message into a constant improves maintainability by making it easier to update the message in the future, but it is not a critical change.

    7
    Best practice
    Use a more specific logger name

    Consider using a more specific logger name, such as
    'org.openqa.selenium.grid.config.TomlConfig', instead of using the class name
    directly.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [40]

    -private static final Logger LOG = Logger.getLogger(TomlConfig.class.getName());
    +private static final Logger LOG = Logger.getLogger("org.openqa.selenium.grid.config.TomlConfig");
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using a more specific logger name can improve logging clarity and organization, but the current logger name is already functional and this change is not crucial.

    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.

    2 participants