SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.73k stars 8.02k forks source link

[py] property strict_timestamp was added #14168

Closed iampopovich closed 1 week ago

iampopovich commented 1 week ago

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

according to #14143 property was added to configure timestamp mode

Motivation and Context

Types of changes

Checklist


PR Type

Enhancement, Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
firefox_profile.py
Add strict_timestamps property to FirefoxProfile class     

py/selenium/webdriver/firefox/firefox_profile.py
  • Added _strict_timestamps attribute with getter and setter methods.
  • Integrated strict_timestamps property into ZipFile instantiation.
  • +9/-1     

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

    codiumai-pr-agent-pro[bot] commented 1 week ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 2
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The new strict_timestamps property is set to True by default. Ensure this default setting aligns with the existing behavior or expectations of users to avoid breaking changes.
    codiumai-pr-agent-pro[bot] commented 1 week ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a validation check in the setter to ensure the value is a boolean ___ **Consider adding a validation check in the strict_timestamps setter to ensure that the
    value being set is a boolean. This will help prevent potential issues if a non-boolean
    value is assigned.** [py/selenium/webdriver/firefox/firefox_profile.py [101-103]](https://github.com/SeleniumHQ/selenium/pull/14168/files#diff-323be8fb876362e1b98205768bcd1e386d4607db6a5164932f7b34e091a22cc1R101-R103) ```diff @strict_timestamps.setter def strict_timestamps(self, value: bool): + if not isinstance(value, bool): + raise ValueError("strict_timestamps must be a boolean") self._strict_timestamps = value ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding a validation check for boolean type in the setter method for `strict_timestamps` is crucial to prevent runtime errors and ensure type safety, which is a significant improvement.
    8
    Enhancement
    Rename the attribute to make it more descriptive ___ **To improve readability and maintainability, consider renaming the _strict_timestamps
    attribute to _strict_timestamps_enabled to make it more descriptive.** [py/selenium/webdriver/firefox/firefox_profile.py [56]](https://github.com/SeleniumHQ/selenium/pull/14168/files#diff-323be8fb876362e1b98205768bcd1e386d4607db6a5164932f7b34e091a22cc1R56-R56) ```diff -self._strict_timestamps = True +self._strict_timestamps_enabled = True ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Renaming `_strict_timestamps` to `_strict_timestamps_enabled` enhances readability and clarity of the code, making it easier to understand the purpose of the attribute.
    6
    Update property methods to use the new attribute name ___ **To ensure consistency, update the strict_timestamps property methods to use the new
    attribute name _strict_timestamps_enabled.** [py/selenium/webdriver/firefox/firefox_profile.py [98-103]](https://github.com/SeleniumHQ/selenium/pull/14168/files#diff-323be8fb876362e1b98205768bcd1e386d4607db6a5164932f7b34e091a22cc1R98-R103) ```diff def strict_timestamps(self) -> bool: - return self._strict_timestamps + return self._strict_timestamps_enabled @strict_timestamps.setter def strict_timestamps(self, value: bool): - self._strict_timestamps = value + self._strict_timestamps_enabled = value ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: If the attribute name is changed, it is essential to update all references to this attribute to maintain consistency. This suggestion correctly follows the change proposed in suggestion 2.
    6
    Maintainability
    Add a comment to explain the purpose of the new property ___ **Add a brief explanation or comment above the strict_timestamps property to clarify its
    purpose and usage, especially since it is a new addition.** [py/selenium/webdriver/firefox/firefox_profile.py [97-99]](https://github.com/SeleniumHQ/selenium/pull/14168/files#diff-323be8fb876362e1b98205768bcd1e386d4607db6a5164932f7b34e091a22cc1R97-R99) ```diff +# Property to get or set the strict timestamps for the zipfile @property def strict_timestamps(self) -> bool: return self._strict_timestamps ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 5 Why: Adding a comment to explain the new `strict_timestamps` property improves code maintainability by making the codebase more understandable, although it's a relatively minor enhancement.
    5
    codecov[bot] commented 1 week ago

    Codecov Report

    All modified and coverable lines are covered by tests :white_check_mark:

    Project coverage is 57.18%. Comparing base (84828cd) to head (24b92ea).

    :exclamation: Current head 24b92ea differs from pull request most recent head fb52580

    Please upload reports for the commit fb52580 to get more accurate results.

    Additional details and impacted files ```diff @@ Coverage Diff @@ ## trunk #14168 +/- ## ======================================= Coverage 57.18% 57.18% ======================================= Files 89 89 Lines 5514 5514 Branches 232 232 ======================================= Hits 3153 3153 Misses 2129 2129 Partials 232 232 ```

    :umbrella: View full report in Codecov by Sentry.
    :loudspeaker: Have feedback on the report? Share it here.

    milahu commented 1 week ago

    ok. squash commits as something like set strict_timestamps=False in zipfile.ZipFile or disable strict timestamps in ZipFile

    titusfortner commented 1 week ago

    We squash as part of the GitHub merge process. Hmm, I'm not sure why that chrome test is failing in this PR but not on trunk, but at least it is obviously unrelated.

    titusfortner commented 1 week ago

    Thanks @milahu & @iampopovich; sorry that I made a one-line change more difficult than it needed to be.